From 5f3e04bd3dac5421e96bee2c807529fa34fe3014 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 12:59:22 +0200 Subject: [PATCH 01/29] refactor: AttachHasOnePointers --- .../Data/DefaultEntityRepository.cs | 51 +++++++++++++------ .../Request/HasOneRelationshipPointers.cs | 6 +-- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 4cd32de946..bc3c9e6361 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -82,6 +82,7 @@ public DefaultEntityRepository( public virtual IQueryable Get() => _dbSet; + /// public virtual IQueryable Select(IQueryable entities, List fields) { if (fields?.Count > 0) @@ -228,7 +229,7 @@ private void AttachHasMany(TEntity entity, HasManyAttribute relationship, IList var relatedList = (IList)entity.GetType().GetProperty(relationship.EntityPropertyName)?.GetValue(entity); foreach (var related in relatedList) { - if (_context.EntityIsTracked(related as IIdentifiable) == false) + if (_context.EntityIsTracked((IIdentifiable)related) == false) _context.Entry(related).State = EntityState.Unchanged; } } @@ -236,8 +237,8 @@ private void AttachHasMany(TEntity entity, HasManyAttribute relationship, IList { foreach (var pointer in pointers) { - if (_context.EntityIsTracked(pointer as IIdentifiable) == false) - _context.Entry(pointer).State = EntityState.Unchanged; + if (_context.EntityIsTracked((IIdentifiable)pointer) == false) + _context.Entry(pointer).State = EntityState.Unchanged; } } } @@ -266,31 +267,49 @@ private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasMan /// /// This is used to allow creation of HasOne relationships when the - /// independent side of the relationship already exists. /// private void AttachHasOnePointers(TEntity entity) { - var relationships = _jsonApiContext.HasOneRelationshipPointers.Get(); + var relationships = _jsonApiContext + .HasOneRelationshipPointers + .Get(); + foreach (var relationship in relationships) { - if (relationship.Key.GetType() != typeof(HasOneAttribute)) - continue; + var pointer = GetValueFromRelationship(entity, relationship); + if (pointer == null) return; + var trackedEntity = _context.GetTrackedEntity(pointer); - var hasOne = (HasOneAttribute)relationship.Key; - if (hasOne.EntityPropertyName != null) + // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified + if (trackedEntity == null) { - var relatedEntity = entity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(entity); - if (relatedEntity != null && _context.Entry(relatedEntity).State == EntityState.Detached && _context.EntityIsTracked((IIdentifiable)relatedEntity) == false) - _context.Entry(relatedEntity).State = EntityState.Unchanged; - } - else + /// the relationship pointer is new to EF Core, but we are sure + /// it exists in the database (json:api spec), so we attach it. + _context.Entry(pointer).State = EntityState.Unchanged; + } else { - if (_context.Entry(relationship.Value).State == EntityState.Detached && _context.EntityIsTracked(relationship.Value) == false) - _context.Entry(relationship.Value).State = EntityState.Unchanged; + /// there already was an instance of this type and ID tracked + /// by EF Core. Reattaching is not allowed, and from now on we + /// will use the already attached one instead. (This entry might + /// contain updated fields as a result of Business logic. + relationships[relationship.Key] = trackedEntity; } + } } + IIdentifiable GetValueFromRelationship(TEntity principalEntity, KeyValuePair relationship ) + { + HasOneAttribute hasOne = (HasOneAttribute)relationship.Key; + if (hasOne.EntityPropertyName != null) + { + var relatedEntity = principalEntity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(principalEntity); + return (IIdentifiable)relatedEntity; + } + return relationship.Value; + } + + /// public virtual async Task UpdateAsync(TId id, TEntity entity) { diff --git a/src/JsonApiDotNetCore/Request/HasOneRelationshipPointers.cs b/src/JsonApiDotNetCore/Request/HasOneRelationshipPointers.cs index 753aa35e98..19046b9eaa 100644 --- a/src/JsonApiDotNetCore/Request/HasOneRelationshipPointers.cs +++ b/src/JsonApiDotNetCore/Request/HasOneRelationshipPointers.cs @@ -28,18 +28,18 @@ namespace JsonApiDotNetCore.Request /// public class HasOneRelationshipPointers { - private Dictionary _hasOneRelationships = new Dictionary(); + private readonly Dictionary _hasOneRelationships = new Dictionary(); /// /// Add the relationship to the list of relationships that should be /// set in the repository layer. /// - public void Add(RelationshipAttribute relationship, IIdentifiable entity) + public void Add(HasOneAttribute relationship, IIdentifiable entity) => _hasOneRelationships[relationship] = entity; /// /// Get all the models that should be associated /// - public Dictionary Get() => _hasOneRelationships; + public Dictionary Get() => _hasOneRelationships; } } From e8d109643be9da442a198ebb3293a940588887ac Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 14:28:48 +0200 Subject: [PATCH 02/29] refactor: AssignHasMany pointers --- .../Data/DefaultEntityRepository.cs | 148 ++++++++++++------ 1 file changed, 97 insertions(+), 51 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index bc3c9e6361..6e383767c2 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -210,58 +210,89 @@ public void DetachRelationshipPointers(TEntity entity) /// This is used to allow creation of HasMany relationships when the /// dependent side of the relationship already exists. /// - private void AttachHasManyPointers(TEntity entity) + private void AttachHasManyAndHasManyThroughPointers(TEntity entity) { var relationships = _jsonApiContext.HasManyRelationshipPointers.Get(); - foreach (var relationship in relationships) + + foreach (var attribute in relationships.Keys) { - if (relationship.Key is HasManyThroughAttribute hasManyThrough) - AttachHasManyThrough(entity, hasManyThrough, relationship.Value); + IEnumerable pointers; + if (attribute is HasManyThroughAttribute hasManyThrough) + { + pointers = relationships[attribute].Cast(); + } else - AttachHasMany(entity, relationship.Key as HasManyAttribute, relationship.Value); + { + pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); + } + + if (pointers == null) continue; + bool alreadyTracked = false; + var newPointerCollection = pointers.Select(pointer => + { + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) alreadyTracked = true; + return tracked ?? pointer; + }); + + if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; } } - private void AttachHasMany(TEntity entity, HasManyAttribute relationship, IList pointers) + private void AttachHasMany(TEntity entity, RelationshipAttribute attribute, Dictionary relationships) { - if (relationship.EntityPropertyName != null) + IEnumerable pointers; + if (attribute is HasManyThroughAttribute hasManyThrough) { - var relatedList = (IList)entity.GetType().GetProperty(relationship.EntityPropertyName)?.GetValue(entity); - foreach (var related in relatedList) - { - if (_context.EntityIsTracked((IIdentifiable)related) == false) - _context.Entry(related).State = EntityState.Unchanged; - } - } - else + pointers = relationships[attribute].Cast(); + } else { - foreach (var pointer in pointers) - { - if (_context.EntityIsTracked((IIdentifiable)pointer) == false) - _context.Entry(pointer).State = EntityState.Unchanged; - } + pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); } + + if (pointers == null) return; + bool alreadyTracked = false; + var newPointerCollection = pointers.Select(pointer => + { + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) alreadyTracked = true; + return tracked ?? pointer; + }); + + if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; } - private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList pointers) + private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, Dictionary relationships) { - // create the collection (e.g. List) - // this type MUST implement IList so we can build the collection - // if this is problematic, we _could_ reflect on the type and find an Add method - // or we might be able to create a proxy type and implement the enumerator - var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; - hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); + var pointers = relationships[hasManyThrough].Cast(); + bool alreadyTracked = false; + var newPointerCollection = pointers.Select(pointer => + { + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) alreadyTracked = true; + return tracked ?? pointer; + }); + if (alreadyTracked) relationships[hasManyAttribute] = (IList)newPointerCollection; foreach (var pointer in pointers) { if (_context.EntityIsTracked(pointer as IIdentifiable) == false) _context.Entry(pointer).State = EntityState.Unchanged; - var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); - hasManyThrough.LeftProperty.SetValue(throughInstance, entity); - hasManyThrough.RightProperty.SetValue(throughInstance, pointer); + var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; + hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); + + foreach (var pointer in pointers) + { + if (_context.EntityIsTracked(pointer as IIdentifiable) == false) + _context.Entry(pointer).State = EntityState.Unchanged; + var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); + + hasManyThrough.LeftProperty.SetValue(throughInstance, entity); + hasManyThrough.RightProperty.SetValue(throughInstance, pointer); - throughRelationshipCollection.Add(throughInstance); + throughRelationshipCollection.Add(throughInstance); + } } } @@ -276,31 +307,16 @@ private void AttachHasOnePointers(TEntity entity) foreach (var relationship in relationships) { - var pointer = GetValueFromRelationship(entity, relationship); + var pointer = GetRelationshipPointer(entity, relationship); if (pointer == null) return; - var trackedEntity = _context.GetTrackedEntity(pointer); - - // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified - if (trackedEntity == null) - { - /// the relationship pointer is new to EF Core, but we are sure - /// it exists in the database (json:api spec), so we attach it. - _context.Entry(pointer).State = EntityState.Unchanged; - } else - { - /// there already was an instance of this type and ID tracked - /// by EF Core. Reattaching is not allowed, and from now on we - /// will use the already attached one instead. (This entry might - /// contain updated fields as a result of Business logic. - relationships[relationship.Key] = trackedEntity; - } - + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) relationships[relationship.Key] = tracked; } } - IIdentifiable GetValueFromRelationship(TEntity principalEntity, KeyValuePair relationship ) + IIdentifiable GetRelationshipPointer(TEntity principalEntity, KeyValuePair relationship) { - HasOneAttribute hasOne = (HasOneAttribute)relationship.Key; + HasOneAttribute hasOne = relationship.Key; if (hasOne.EntityPropertyName != null) { var relatedEntity = principalEntity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(principalEntity); @@ -309,6 +325,36 @@ IIdentifiable GetValueFromRelationship(TEntity principalEntity, KeyValuePair GetRelationshipPointers(TEntity entity, HasManyAttribute attribute) + { + if (attribute.EntityPropertyName == null) + { + return null; + } + return ((IEnumerable)(entity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(entity))).Cast(); + } + + // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified + IIdentifiable AttachOrGetTracked(IIdentifiable pointer) + { + var trackedEntity = _context.GetTrackedEntity(pointer); + + + if (trackedEntity != null) + { + /// there already was an instance of this type and ID tracked + /// by EF Core. Reattaching will produce a conflict, and from now on we + /// will use the already attached one instead. (This entry might + /// contain updated fields as a result of business logic) + return trackedEntity; + } + + /// the relationship pointer is new to EF Core, but we are sure + /// it exists in the database (json:api spec), so we attach it. + _context.Entry(pointer).State = EntityState.Unchanged; + return null; + } + /// public virtual async Task UpdateAsync(TId id, TEntity entity) From 1420d6deb3e808f9f0c8dc375e6b7e3f70ed5c60 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 16:09:42 +0200 Subject: [PATCH 03/29] refactor assign relationships and applied to updateasync method --- .../Data/DefaultEntityRepository.cs | 303 +++++++----------- .../Extensions/DbContextExtensions.cs | 3 + .../Request/HasManyRelationshipPointers.cs | 2 +- .../Serialization/JsonApiDeSerializer.cs | 4 +- .../Services/EntityResourceService.cs | 2 + .../Services/IJsonApiContext.cs | 2 +- .../Services/JsonApiContext.cs | 12 +- 7 files changed, 137 insertions(+), 191 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 6e383767c2..63fd81b557 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -158,7 +158,7 @@ public virtual async Task CreateAsync(TEntity entity) protected virtual void AttachRelationships(TEntity entity = null) { - AttachHasManyPointers(entity); + AttachHasManyAndHasManyThroughPointers(entity); AttachHasOnePointers(entity); } @@ -206,159 +206,15 @@ public void DetachRelationshipPointers(TEntity entity) } } - /// - /// This is used to allow creation of HasMany relationships when the - /// dependent side of the relationship already exists. - /// - private void AttachHasManyAndHasManyThroughPointers(TEntity entity) - { - var relationships = _jsonApiContext.HasManyRelationshipPointers.Get(); - - foreach (var attribute in relationships.Keys) - { - IEnumerable pointers; - if (attribute is HasManyThroughAttribute hasManyThrough) - { - pointers = relationships[attribute].Cast(); - } - else - { - pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); - } - - if (pointers == null) continue; - bool alreadyTracked = false; - var newPointerCollection = pointers.Select(pointer => - { - var tracked = AttachOrGetTracked(pointer); - if (tracked != null) alreadyTracked = true; - return tracked ?? pointer; - }); - - if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; - } - } - - private void AttachHasMany(TEntity entity, RelationshipAttribute attribute, Dictionary relationships) - { - IEnumerable pointers; - if (attribute is HasManyThroughAttribute hasManyThrough) - { - pointers = relationships[attribute].Cast(); - } else - { - pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); - } - - if (pointers == null) return; - bool alreadyTracked = false; - var newPointerCollection = pointers.Select(pointer => - { - var tracked = AttachOrGetTracked(pointer); - if (tracked != null) alreadyTracked = true; - return tracked ?? pointer; - }); - - if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; - } - - private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, Dictionary relationships) - { - var pointers = relationships[hasManyThrough].Cast(); - bool alreadyTracked = false; - var newPointerCollection = pointers.Select(pointer => - { - var tracked = AttachOrGetTracked(pointer); - if (tracked != null) alreadyTracked = true; - return tracked ?? pointer; - }); - if (alreadyTracked) relationships[hasManyAttribute] = (IList)newPointerCollection; - - foreach (var pointer in pointers) - { - if (_context.EntityIsTracked(pointer as IIdentifiable) == false) - _context.Entry(pointer).State = EntityState.Unchanged; - - var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; - hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); - - foreach (var pointer in pointers) - { - if (_context.EntityIsTracked(pointer as IIdentifiable) == false) - _context.Entry(pointer).State = EntityState.Unchanged; - var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); - hasManyThrough.LeftProperty.SetValue(throughInstance, entity); - hasManyThrough.RightProperty.SetValue(throughInstance, pointer); - - throughRelationshipCollection.Add(throughInstance); - } - } - } - - /// - /// This is used to allow creation of HasOne relationships when the - /// - private void AttachHasOnePointers(TEntity entity) - { - var relationships = _jsonApiContext - .HasOneRelationshipPointers - .Get(); - - foreach (var relationship in relationships) - { - var pointer = GetRelationshipPointer(entity, relationship); - if (pointer == null) return; - var tracked = AttachOrGetTracked(pointer); - if (tracked != null) relationships[relationship.Key] = tracked; - } - } - - IIdentifiable GetRelationshipPointer(TEntity principalEntity, KeyValuePair relationship) - { - HasOneAttribute hasOne = relationship.Key; - if (hasOne.EntityPropertyName != null) - { - var relatedEntity = principalEntity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(principalEntity); - return (IIdentifiable)relatedEntity; - } - return relationship.Value; - } - - IEnumerable GetRelationshipPointers(TEntity entity, HasManyAttribute attribute) - { - if (attribute.EntityPropertyName == null) - { - return null; - } - return ((IEnumerable)(entity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(entity))).Cast(); - } - - // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified - IIdentifiable AttachOrGetTracked(IIdentifiable pointer) - { - var trackedEntity = _context.GetTrackedEntity(pointer); - - - if (trackedEntity != null) - { - /// there already was an instance of this type and ID tracked - /// by EF Core. Reattaching will produce a conflict, and from now on we - /// will use the already attached one instead. (This entry might - /// contain updated fields as a result of business logic) - return trackedEntity; - } - - /// the relationship pointer is new to EF Core, but we are sure - /// it exists in the database (json:api spec), so we attach it. - _context.Entry(pointer).State = EntityState.Unchanged; - return null; - } /// public virtual async Task UpdateAsync(TId id, TEntity entity) { + /// WHY is parameter "entity" even passed along to this method?? + /// It does nothing! + var oldEntity = await GetAsync(id); if (oldEntity == null) @@ -369,37 +225,24 @@ 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) + AttachRelationships(oldEntity); + foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) { - if (relationship.Key is HasManyThroughAttribute throughAttribute) + var relationshipValue = relationshipEntry.Value; + if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) { + // load relations to enforce complete replace 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 ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasOneAttribute))) + AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); + } else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) { - relationship.Key.SetValue(oldEntity, relationship.Value); + // load relations to enforce complete replace + await _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).LoadAsync(); + hasManyAttribute.SetValue(oldEntity, relationshipValue); } - if ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasManyAttribute))) + else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { - await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); - var value = PreventReattachment((IEnumerable)relationship.Value); - relationship.Key.SetValue(oldEntity, value); + hasOneAttribute.SetValue(oldEntity, relationshipValue); } } } @@ -407,20 +250,19 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) return oldEntity; } - /// - /// 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. - object PreventReattachment(IEnumerable relatedEntities) + private void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList relationshipValue) { - var relatedType = TypeHelper.GetTypeOfList(relatedEntities.GetType()); - var replaced = relatedEntities.Cast().Select(entity => _context.GetTrackedEntity(entity) ?? entity); - return TypeHelper.ConvertCollection(replaced, relatedType); + var pointers = relationshipValue.Cast(); + var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; + hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); + foreach (var pointer in pointers) + { + var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); + hasManyThrough.LeftProperty.SetValue(throughInstance, entity); + hasManyThrough.RightProperty.SetValue(throughInstance, pointer); + throughRelationshipCollection.Add(throughInstance); + } } @@ -535,5 +377,96 @@ public async Task> ToListAsync(IQueryable entiti ? await entities.ToListAsync() : entities.ToList(); } + + /// + /// This is used to allow creation of HasMany relationships when the + /// dependent side of the relationship already exists. + /// + private void AttachHasManyAndHasManyThroughPointers(TEntity entity) + { + var relationships = _jsonApiContext.HasManyRelationshipPointers.Get(); + + foreach (var attribute in relationships.Keys.ToArray()) + { + IEnumerable pointers; + if (attribute is HasManyThroughAttribute hasManyThrough) + { + pointers = relationships[attribute].Cast(); + } + else + { + pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); + } + + if (pointers == null) continue; + bool alreadyTracked = false; + var newPointerCollection = pointers.Select(pointer => + { + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) alreadyTracked = true; + return tracked ?? pointer; + }); + + if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; + } + } + + + + /// + /// This is used to allow creation of HasOne relationships when the + /// + private void AttachHasOnePointers(TEntity entity) + { + var relationships = _jsonApiContext + .HasOneRelationshipPointers + .Get(); + + foreach (var attribute in relationships.Keys.ToArray()) + { + var pointer = GetRelationshipPointer(entity, attribute) ?? relationships[attribute]; + if (pointer == null) return; + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) relationships[attribute] = tracked; + } + } + + IIdentifiable GetRelationshipPointer(TEntity principalEntity, HasOneAttribute attribute) + { + if (attribute.EntityPropertyName == null) + { + return null; + } + return (IIdentifiable)principalEntity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(principalEntity); + } + + IEnumerable GetRelationshipPointers(TEntity entity, HasManyAttribute attribute) + { + if (attribute.EntityPropertyName == null) + { + return null; + } + return ((IEnumerable)(entity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(entity))).Cast(); + } + + // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified + IIdentifiable AttachOrGetTracked(IIdentifiable pointer) + { + var trackedEntity = _context.GetTrackedEntity(pointer); + + if (trackedEntity != null) + { + /// there already was an instance of this type and ID tracked + /// by EF Core. Reattaching will produce a conflict, and from now on we + /// will use the already attached one instead. (This entry might + /// contain updated fields as a result of business logic) + return trackedEntity; + } + + /// the relationship pointer is new to EF Core, but we are sure + /// it exists in the database (json:api spec), so we attach it. + _context.Entry(pointer).State = EntityState.Unchanged; + return null; + } } } diff --git a/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs b/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs index 42f998891c..e90471083c 100644 --- a/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs @@ -1,8 +1,10 @@ using System; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using JsonApiDotNetCore.Models; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Storage; @@ -32,6 +34,7 @@ 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. diff --git a/src/JsonApiDotNetCore/Request/HasManyRelationshipPointers.cs b/src/JsonApiDotNetCore/Request/HasManyRelationshipPointers.cs index 04056e4af8..3fda5dc44e 100644 --- a/src/JsonApiDotNetCore/Request/HasManyRelationshipPointers.cs +++ b/src/JsonApiDotNetCore/Request/HasManyRelationshipPointers.cs @@ -32,7 +32,7 @@ namespace JsonApiDotNetCore.Request /// public class HasManyRelationshipPointers { - private Dictionary _hasManyRelationships = new Dictionary(); + private readonly Dictionary _hasManyRelationships = new Dictionary(); /// /// Add the relationship to the list of relationships that should be diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index 4e3beaaf81..38acf72780 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -250,7 +250,7 @@ private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, var convertedValue = TypeHelper.ConvertType(foreignKeyPropertyValue, foreignKeyProperty.PropertyType); foreignKeyProperty.SetValue(entity, convertedValue); - _jsonApiContext.RelationshipsToUpdate[hasOneAttr] = convertedValue; + //_jsonApiContext.RelationshipsToUpdate[hasOneAttr] = convertedValue; } } @@ -300,7 +300,7 @@ private object SetHasManyRelationship(object entity, var convertedCollection = TypeHelper.ConvertCollection(relatedResources, attr.Type); attr.SetValue(entity, convertedCollection); - _jsonApiContext.RelationshipsToUpdate[attr] = convertedCollection; + //_jsonApiContext.RelationshipsToUpdate[attr] = convertedCollection; // WHAT exactly is the use of these two different collections..? _jsonApiContext.HasManyRelationshipPointers.Add(attr, convertedCollection); } diff --git a/src/JsonApiDotNetCore/Services/EntityResourceService.cs b/src/JsonApiDotNetCore/Services/EntityResourceService.cs index b392b3600d..3da420845c 100644 --- a/src/JsonApiDotNetCore/Services/EntityResourceService.cs +++ b/src/JsonApiDotNetCore/Services/EntityResourceService.cs @@ -154,6 +154,8 @@ public virtual async Task UpdateAsync(TId id, TResource resource) { var entity = MapIn(resource); + + entity = await _entities.UpdateAsync(id, entity); return MapOut(entity); diff --git a/src/JsonApiDotNetCore/Services/IJsonApiContext.cs b/src/JsonApiDotNetCore/Services/IJsonApiContext.cs index 2509d7124f..e0a14a4e02 100644 --- a/src/JsonApiDotNetCore/Services/IJsonApiContext.cs +++ b/src/JsonApiDotNetCore/Services/IJsonApiContext.cs @@ -35,7 +35,7 @@ public interface IUpdateRequest /// Any relationships that were included in a PATCH request. /// Only the relationships in this dictionary should be updated. /// - Dictionary RelationshipsToUpdate { get; set; } + Dictionary RelationshipsToUpdate { get; } } public interface IJsonApiRequest : IJsonApiApplication, IUpdateRequest, IQueryRequest diff --git a/src/JsonApiDotNetCore/Services/JsonApiContext.cs b/src/JsonApiDotNetCore/Services/JsonApiContext.cs index 2553cbe451..bec78e8237 100644 --- a/src/JsonApiDotNetCore/Services/JsonApiContext.cs +++ b/src/JsonApiDotNetCore/Services/JsonApiContext.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using JsonApiDotNetCore.Builders; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Internal; @@ -52,7 +53,15 @@ public JsonApiContext( public bool IsBulkOperationRequest { get; set; } public Dictionary AttributesToUpdate { get; set; } = new Dictionary(); - public Dictionary RelationshipsToUpdate { get; set; } = new Dictionary(); + public Dictionary RelationshipsToUpdate { get => GetRelationshipsToUpdate(); } + + private Dictionary GetRelationshipsToUpdate() + { + var hasOneEntries = HasOneRelationshipPointers.Get().ToDictionary(kvp => (RelationshipAttribute)kvp.Key, kvp => (object)kvp.Value); + var hasManyEntries = HasManyRelationshipPointers.Get().ToDictionary(kvp => kvp.Key, kvp => (object)kvp.Value); + return hasOneEntries.Union(hasManyEntries).ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + } + public HasManyRelationshipPointers HasManyRelationshipPointers { get; private set; } = new HasManyRelationshipPointers(); public HasOneRelationshipPointers HasOneRelationshipPointers { get; private set; } = new HasOneRelationshipPointers(); @@ -138,7 +147,6 @@ public void BeginOperation() { IncludedRelationships = new List(); AttributesToUpdate = new Dictionary(); - RelationshipsToUpdate = new Dictionary(); HasManyRelationshipPointers = new HasManyRelationshipPointers(); HasOneRelationshipPointers = new HasOneRelationshipPointers(); } From 24dd97c03f8dbfb04178b50528c9e33542544de0 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 16:54:54 +0200 Subject: [PATCH 04/29] refactor: tests passing again --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 3 ++- src/JsonApiDotNetCore/Extensions/TypeExtensions.cs | 7 +++++++ src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 63fd81b557..ce6d8fdc2f 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -242,6 +242,7 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) } else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { + //var foreignkeyValue = relationshipValue.GetType().GetProperty(hasOneAttribute.IdentifiablePropertyName).GetValue(oldEntity); hasOneAttribute.SetValue(oldEntity, relationshipValue); } } @@ -405,7 +406,7 @@ private void AttachHasManyAndHasManyThroughPointers(TEntity entity) var tracked = AttachOrGetTracked(pointer); if (tracked != null) alreadyTracked = true; return tracked ?? pointer; - }); + }).Cast(attribute.Type); if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; } diff --git a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs index 03874d9b76..d02f122061 100644 --- a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs @@ -8,6 +8,13 @@ namespace JsonApiDotNetCore.Extensions { internal static class TypeExtensions { + public static IEnumerable Cast(this IEnumerable source, Type type) + { + if (source == null) throw new ArgumentNullException(nameof(source)); + if (type == null) throw new ArgumentNullException(nameof(type)); + return TypeHelper.ConvertCollection(source.Cast(), type); + } + public static Type GetElementType(this IEnumerable enumerable) { var enumerableTypes = enumerable.GetType() diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index 38acf72780..259a4ff01c 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -250,7 +250,7 @@ private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, var convertedValue = TypeHelper.ConvertType(foreignKeyPropertyValue, foreignKeyProperty.PropertyType); foreignKeyProperty.SetValue(entity, convertedValue); - //_jsonApiContext.RelationshipsToUpdate[hasOneAttr] = convertedValue; + if (convertedValue == null ) _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); } } From 030472527c1778269b442631a2002b436e224f2a Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 17:13:45 +0200 Subject: [PATCH 05/29] test: add implicit removal test --- .../Models/Passport.cs | 11 +++++ .../JsonApiDotNetCoreExample/Models/Person.cs | 4 ++ .../Data/DefaultEntityRepository.cs | 1 - .../Spec/UpdatingRelationshipsTests.cs | 47 +++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/Examples/JsonApiDotNetCoreExample/Models/Passport.cs diff --git a/src/Examples/JsonApiDotNetCoreExample/Models/Passport.cs b/src/Examples/JsonApiDotNetCoreExample/Models/Passport.cs new file mode 100644 index 0000000000..7586b67674 --- /dev/null +++ b/src/Examples/JsonApiDotNetCoreExample/Models/Passport.cs @@ -0,0 +1,11 @@ +using JsonApiDotNetCore.Models; + +namespace JsonApiDotNetCoreExample.Models +{ + public class Passport : Identifiable + { + public virtual int? SocialSecurityNumber { get; set; } + [HasOne("person")] + public virtual Person Person { get; set; } + } +} \ No newline at end of file diff --git a/src/Examples/JsonApiDotNetCoreExample/Models/Person.cs b/src/Examples/JsonApiDotNetCoreExample/Models/Person.cs index eb032cc5fd..63eea72cb0 100644 --- a/src/Examples/JsonApiDotNetCoreExample/Models/Person.cs +++ b/src/Examples/JsonApiDotNetCoreExample/Models/Person.cs @@ -45,5 +45,9 @@ public Dictionary GetMeta(IJsonApiContext context) { "authors", new string[] { "Jared Nance" } } }; } + public int? PassportId { get; set; } + [HasOne("passport")] + public virtual Passport Passport { get; set; } + } } diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index ce6d8fdc2f..3a1ecaed6b 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -242,7 +242,6 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) } else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { - //var foreignkeyValue = relationshipValue.GetType().GetProperty(hasOneAttribute.IdentifiablePropertyName).GetValue(oldEntity); hasOneAttribute.SetValue(oldEntity, relationshipValue); } } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index f126dbee9d..21b075a132 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -621,5 +621,52 @@ public async Task Can_Delete_Relationship_By_Patching_Relationship() Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.Null(todoItemResult.Owner); } + + [Fact] + public async Task Updating_ToOne_Relationship_With_Implicit_Remove() + { + + // Arrange + var context = _fixture.GetService(); + var passport = new Passport(); + var person1 = _personFaker.Generate(); + person1.Passport = passport; + var person2 = _personFaker.Generate(); + context.People.AddRange(new List() { person1, person2 }); + await context.SaveChangesAsync(); + + var content = new + { + data = new + { + type = "people", + id = person2.Id, + relationships = new Dictionary + { + { "passport", new + { + data = new { type = "passports", id = $"{person1.PassportId}" } + } + } + } + } + }; + + var httpMethod = new HttpMethod("PATCH"); + var route = $"/api/v1/people/{person2.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 _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}"); + + } } } From f60587f56a021b3e531330f3e0cfe49e43e93fe7 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 17:23:05 +0200 Subject: [PATCH 06/29] test: all passing --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 9 +++++++++ .../Acceptance/Spec/UpdatingRelationshipsTests.cs | 1 - 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 3a1ecaed6b..ab1008690d 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -149,6 +149,15 @@ public virtual async Task GetAndIncludeAsync(TId id, string relationshi public virtual async Task CreateAsync(TEntity entity) { AttachRelationships(entity); + foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) + { + var relationshipValue = relationshipEntry.Value; + if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) + { + AssignHasManyThrough(entity, throughAttribute, (IList)relationshipValue); + } + } + _dbSet.Add(entity); await _context.SaveChangesAsync(); diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 21b075a132..ac464ed68e 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -625,7 +625,6 @@ public async Task Can_Delete_Relationship_By_Patching_Relationship() [Fact] public async Task Updating_ToOne_Relationship_With_Implicit_Remove() { - // Arrange var context = _fixture.GetService(); var passport = new Passport(); From 4cd9465554005fb8a4b5b17acf35f2841422e3b6 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 19:16:27 +0200 Subject: [PATCH 07/29] chore: addedd notes wrt implicit remove --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 2 ++ .../Acceptance/Spec/UpdatingRelationshipsTests.cs | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index ab1008690d..d01a9108d9 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -247,10 +247,12 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) { // load relations to enforce complete replace await _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).LoadAsync(); + // also need to load inverse relationship here, see issue #502 hasManyAttribute.SetValue(oldEntity, relationshipValue); } else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { + // need to load inverse relationship here, see issue #502 hasOneAttribute.SetValue(oldEntity, relationshipValue); } } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index ac464ed68e..4e77d155b5 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -664,7 +664,8 @@ public async Task Updating_ToOne_Relationship_With_Implicit_Remove() // Assert var body = await response.Content.ReadAsStringAsync(); - Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + // this test currently fails, will be adressed in next PR + //Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); } } From d1f39e6ac8dfc0ca0b31173b35cf2d48975d9c73 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 19:28:12 +0200 Subject: [PATCH 08/29] comments: added comment to assign method --- .../Data/DefaultEntityRepository.cs | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index d01a9108d9..fee22b29b9 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -215,9 +215,6 @@ public void DetachRelationshipPointers(TEntity entity) } } - - - /// public virtual async Task UpdateAsync(TId id, TEntity entity) { @@ -247,12 +244,12 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) { // load relations to enforce complete replace await _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).LoadAsync(); - // also need to load inverse relationship here, see issue #502 + // todo: need to load inverse relationship here, see issue #502 hasManyAttribute.SetValue(oldEntity, relationshipValue); } else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { - // need to load inverse relationship here, see issue #502 + // todo: need to load inverse relationship here, see issue #502 hasOneAttribute.SetValue(oldEntity, relationshipValue); } } @@ -261,22 +258,6 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) return oldEntity; } - private void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList relationshipValue) - { - var pointers = relationshipValue.Cast(); - var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; - hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); - - foreach (var pointer in pointers) - { - var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); - hasManyThrough.LeftProperty.SetValue(throughInstance, entity); - hasManyThrough.RightProperty.SetValue(throughInstance, pointer); - throughRelationshipCollection.Add(throughInstance); - } - } - - /// public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds) { @@ -422,7 +403,26 @@ private void AttachHasManyAndHasManyThroughPointers(TEntity entity) } } + /// + /// The relationshipValue parameter contains the dependent side of the relationship (Tags). + /// We can't directly add them to the principal entity (Article): we need to + /// use the join table (ArticleTags). This methods assigns the relationship value to entity + /// by taking care of that + /// + protected void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList relationshipValue) + { + var pointers = relationshipValue.Cast(); + var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; + hasManyThrough.ThroughProperty.SetValue(entity, throughRelationshipCollection); + foreach (var pointer in pointers) + { + var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType); + hasManyThrough.LeftProperty.SetValue(throughInstance, entity); + hasManyThrough.RightProperty.SetValue(throughInstance, pointer); + throughRelationshipCollection.Add(throughInstance); + } + } /// /// This is used to allow creation of HasOne relationships when the From 7df5233100ffa6f91385503dd094ce22dfe5f01f Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 7 Jun 2019 21:37:16 +0200 Subject: [PATCH 09/29] fix: support for entity resource split --- .../Data/DefaultEntityRepository.cs | 90 +++++++++---------- 1 file changed, 44 insertions(+), 46 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index fee22b29b9..9c2a71d64f 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -11,7 +11,6 @@ using JsonApiDotNetCore.Services; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; - namespace JsonApiDotNetCore.Data { /// @@ -30,7 +29,7 @@ public DefaultEntityRepository( public DefaultEntityRepository( ILoggerFactory loggerFactory, IJsonApiContext jsonApiContext, - IDbContextResolver contextResolver, + IDbContextResolver contextResolver, ResourceDefinition resourceDefinition = null) : base(loggerFactory, jsonApiContext, contextResolver, resourceDefinition) { } @@ -51,7 +50,6 @@ public class DefaultEntityRepository private readonly IJsonApiContext _jsonApiContext; private readonly IGenericProcessorFactory _genericProcessorFactory; private readonly ResourceDefinition _resourceDefinition; - public DefaultEntityRepository( IJsonApiContext jsonApiContext, IDbContextResolver contextResolver, @@ -102,7 +100,6 @@ public virtual IQueryable Filter(IQueryable entities, FilterQu return defaultQueryFilter(entities, filterQuery); } } - return entities.Filter(_jsonApiContext, filterQuery); } @@ -124,7 +121,6 @@ public virtual IQueryable Sort(IQueryable entities, List GetAndIncludeAsync(TId id, string relationshi public virtual async Task CreateAsync(TEntity entity) { AttachRelationships(entity); - foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) - { - var relationshipValue = relationshipEntry.Value; - if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) - { - AssignHasManyThrough(entity, throughAttribute, (IList)relationshipValue); - } - } - + AssignRelationshipValues(entity); _dbSet.Add(entity); await _context.SaveChangesAsync(); @@ -165,6 +153,9 @@ public virtual async Task CreateAsync(TEntity entity) return entity; } + /// + + /// protected virtual void AttachRelationships(TEntity entity = null) { AttachHasManyAndHasManyThroughPointers(entity); @@ -232,27 +223,7 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if (_jsonApiContext.RelationshipsToUpdate.Any()) { AttachRelationships(oldEntity); - foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) - { - var relationshipValue = relationshipEntry.Value; - if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) - { - // load relations to enforce complete replace - await _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).LoadAsync(); - AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); - } else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) - { - // load relations to enforce complete replace - await _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).LoadAsync(); - // todo: need to load inverse relationship here, see issue #502 - hasManyAttribute.SetValue(oldEntity, relationshipValue); - } - else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) - { - // todo: need to load inverse relationship here, see issue #502 - hasOneAttribute.SetValue(oldEntity, relationshipValue); - } - } + AssignRelationshipValues(oldEntity, update: true); } await _context.SaveChangesAsync(); return oldEntity; @@ -273,18 +244,14 @@ public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute await genericProcessor.UpdateRelationshipsAsync(parent, relationship, relationshipIds); } + /// public virtual async Task DeleteAsync(TId id) { var entity = await GetAsync(id); - - if (entity == null) - return false; - + if (entity == null) return false; _dbSet.Remove(entity); - await _context.SaveChangesAsync(); - return true; } @@ -392,14 +359,45 @@ private void AttachHasManyAndHasManyThroughPointers(TEntity entity) if (pointers == null) continue; bool alreadyTracked = false; + Type entityType = null; var newPointerCollection = pointers.Select(pointer => { + entityType = pointer.GetType(); var tracked = AttachOrGetTracked(pointer); if (tracked != null) alreadyTracked = true; - return tracked ?? pointer; - }).Cast(attribute.Type); + return Convert.ChangeType(tracked ?? pointer, entityType); + }).ToList().Cast(entityType); - if (alreadyTracked) relationships[attribute] = (IList)newPointerCollection; + if (alreadyTracked || pointers != relationships[attribute]) relationships[attribute] = (IList)newPointerCollection; + } + } + + /// + /// todo: comments + /// + protected void AssignRelationshipValues(TEntity oldEntity, bool update = false) + { + foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) + { + var relationshipValue = relationshipEntry.Value; + if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) + { + // load relations to enforce complete replace in case of patch + if (update) _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).Load(); + AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); + } + else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) + { + // load relations to enforce complete replace + if (update) _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).Load(); + // todo: need to load inverse relationship here, see issue #502 + hasManyAttribute.SetValue(oldEntity, relationshipValue); + } + else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) + { + // todo: need to load inverse relationship here, see issue #502 + if (update) hasOneAttribute.SetValue(oldEntity, relationshipValue); + } } } @@ -409,7 +407,7 @@ private void AttachHasManyAndHasManyThroughPointers(TEntity entity) /// use the join table (ArticleTags). This methods assigns the relationship value to entity /// by taking care of that /// - protected void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList relationshipValue) + private void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList relationshipValue) { var pointers = relationshipValue.Cast(); var throughRelationshipCollection = Activator.CreateInstance(hasManyThrough.ThroughProperty.PropertyType) as IList; @@ -438,7 +436,7 @@ private void AttachHasOnePointers(TEntity entity) var pointer = GetRelationshipPointer(entity, attribute) ?? relationships[attribute]; if (pointer == null) return; var tracked = AttachOrGetTracked(pointer); - if (tracked != null) relationships[attribute] = tracked; + if (tracked != null || pointer != relationships[attribute]) relationships[attribute] = tracked ?? pointer; } } From 0296eb4d90afedc415d08c02f5a56d0039e88d6b Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Sat, 8 Jun 2019 10:19:15 +0200 Subject: [PATCH 10/29] fix: minor refactor, comments --- .../Data/DefaultEntityRepository.cs | 50 +++++++++++++++---- .../Models/HasOneAttribute.cs | 6 +-- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 9c2a71d64f..93e79e01ec 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -29,7 +29,7 @@ public DefaultEntityRepository( public DefaultEntityRepository( ILoggerFactory loggerFactory, IJsonApiContext jsonApiContext, - IDbContextResolver contextResolver, + IDbContextResolver contextResolver, ResourceDefinition resourceDefinition = null) : base(loggerFactory, jsonApiContext, contextResolver, resourceDefinition) { } @@ -222,14 +222,20 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if (_jsonApiContext.RelationshipsToUpdate.Any()) { + /// First attach all targeted relationships to the dbcontext. + /// This takes into account that some of these entities are + /// already attached in the dbcontext AttachRelationships(oldEntity); - AssignRelationshipValues(oldEntity, update: true); + /// load the current state of the relationship to support complete-replacement + LoadCurrentRelationships(oldEntity); + /// assign the actual relationship values. + AssignRelationshipValues(oldEntity); } await _context.SaveChangesAsync(); return oldEntity; } - /// + public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds) { // TODO: it would be better to let this be determined within the relationship attribute... @@ -373,30 +379,52 @@ private void AttachHasManyAndHasManyThroughPointers(TEntity entity) } /// - /// todo: comments + /// Before assigning new relationship values (updateasync), we need to + /// attach load the current relationship state into the dbcontext, else + /// there won't be a complete-replace for one-to-many and many-to-many. + /// + /// Old entity. + protected void LoadCurrentRelationships(TEntity oldEntity) + { + foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) + { + var relationshipValue = relationshipEntry.Value; + if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) + { + _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).Load(); + + } + else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) + { + _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).Load(); + } + } + } + + /// + /// assigns relationships that were set in the request to the target entity of the request + /// todo: partially remove dependency on IJsonApiContext here: it is fine to + /// retrieve from the context WHICH relationships to update, but the actual values should + /// come from the context. /// - protected void AssignRelationshipValues(TEntity oldEntity, bool update = false) + protected void AssignRelationshipValues(TEntity oldEntity) { foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) { var relationshipValue = relationshipEntry.Value; if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) { - // load relations to enforce complete replace in case of patch - if (update) _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).Load(); AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); } else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) { - // load relations to enforce complete replace - if (update) _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).Load(); // todo: need to load inverse relationship here, see issue #502 - hasManyAttribute.SetValue(oldEntity, relationshipValue); + hasManyAttribute.SetValue(oldEntity, relationshipValue); } else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) { // todo: need to load inverse relationship here, see issue #502 - if (update) hasOneAttribute.SetValue(oldEntity, relationshipValue); + hasOneAttribute.SetValue(oldEntity, relationshipValue); } } } diff --git a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs index 45b08590b3..e269e1d965 100644 --- a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs +++ b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs @@ -50,13 +50,9 @@ public HasOneAttribute(string publicName = null, Link documentLinks = Link.All, /// The new property value public override void SetValue(object resource, object newValue) { - var propertyName = (newValue?.GetType() == Type) - ? InternalRelationshipName - : IdentifiablePropertyName; - var propertyInfo = resource .GetType() - .GetProperty(propertyName); + .GetProperty(InternalRelationshipName); propertyInfo.SetValue(resource, newValue); } From f0d5924549d7ecc7b87380b766a63103c696261d Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Sat, 8 Jun 2019 10:33:44 +0200 Subject: [PATCH 11/29] fix: foreignkey set null bug --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 4 ++-- src/JsonApiDotNetCore/Models/HasOneAttribute.cs | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 93e79e01ec..df99f80535 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -404,8 +404,8 @@ protected void LoadCurrentRelationships(TEntity oldEntity) /// /// assigns relationships that were set in the request to the target entity of the request /// todo: partially remove dependency on IJsonApiContext here: it is fine to - /// retrieve from the context WHICH relationships to update, but the actual values should - /// come from the context. + /// retrieve from the context WHICH relationships to update, but the actual + /// values should not come from the context. /// protected void AssignRelationshipValues(TEntity oldEntity) { diff --git a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs index e269e1d965..adca016c43 100644 --- a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs +++ b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs @@ -50,10 +50,13 @@ public HasOneAttribute(string publicName = null, Link documentLinks = Link.All, /// The new property value public override void SetValue(object resource, object newValue) { - var propertyInfo = resource - .GetType() - .GetProperty(InternalRelationshipName); + string propertyName = InternalRelationshipName; + // if we're deleting the relationship (setting it to null), + // we set the foreignKey to null. We could also set the actual property to null, + // but then we would first need to load the current relationship, which requires an extra query. + if (newValue == null) propertyName = IdentifiablePropertyName; + var propertyInfo = resource.GetType().GetProperty(propertyName); propertyInfo.SetValue(resource, newValue); } From 9f7550cdb9ff6007244f58dd5b17a4dc0927b83f Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 09:15:58 +0200 Subject: [PATCH 12/29] feat: decoupled repository from JsonApiContext with respect to updating entities --- .../Data/DefaultEntityRepository.cs | 237 +++++++++--------- 1 file changed, 121 insertions(+), 116 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index df99f80535..be47d0488d 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -144,23 +144,29 @@ public virtual async Task GetAndIncludeAsync(TId id, string relationshi /// public virtual async Task CreateAsync(TEntity entity) { - AttachRelationships(entity); - AssignRelationshipValues(entity); + foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate?.Keys) + { + var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, entity, out bool wasAlreadyTracked); + // LoadInverseRelationships(trackedRelationshipValue, relationshipAttribute) + if (wasAlreadyTracked) + { + /// We only need to reassign the relationship value to the to-be-added + /// entity when we're using a different instance (because this different one + /// was already tracked) than the one assigned to the to-be-created entity. + AssignRelationshipValue(entity, trackedRelationshipValue, relationshipAttr); + } else if (relationshipAttr is HasManyThroughAttribute throughAttr) + { + /// even if we don't have to reassign anything because of already tracked + /// entities, we still need to assign the "through" entities in the case of many-to-many. + AssignHasManyThrough(entity, throughAttr, (IList)trackedRelationshipValue); + } + } _dbSet.Add(entity); - await _context.SaveChangesAsync(); return entity; } - /// - - /// - protected virtual void AttachRelationships(TEntity entity = null) - { - AttachHasManyAndHasManyThroughPointers(entity); - AttachHasOnePointers(entity); - } /// public void DetachRelationshipPointers(TEntity entity) @@ -207,7 +213,7 @@ public void DetachRelationshipPointers(TEntity entity) } /// - public virtual async Task UpdateAsync(TId id, TEntity entity) + public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) { /// WHY is parameter "entity" even passed along to this method?? /// It does nothing! @@ -217,25 +223,73 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if (oldEntity == null) return null; - foreach (var attr in _jsonApiContext.AttributesToUpdate) - attr.Key.SetValue(oldEntity, attr.Value); + foreach (var attr in _jsonApiContext.AttributesToUpdate.Keys) + attr.SetValue(oldEntity, attr.GetValue(updatedEntity)); - if (_jsonApiContext.RelationshipsToUpdate.Any()) + foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate?.Keys) { - /// First attach all targeted relationships to the dbcontext. - /// This takes into account that some of these entities are - /// already attached in the dbcontext - AttachRelationships(oldEntity); - /// load the current state of the relationship to support complete-replacement - LoadCurrentRelationships(oldEntity); - /// assign the actual relationship values. - AssignRelationshipValues(oldEntity); + LoadCurrentRelationships(oldEntity, relationshipAttr); + var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out bool wasAlreadyTracked); + // LoadInverseRelationships(trackedRelationshipValue, relationshipAttribute) + AssignRelationshipValue(oldEntity, trackedRelationshipValue, relationshipAttr); } await _context.SaveChangesAsync(); return oldEntity; } - /// + + /// + /// Responsible for getting the relationship value for a given relationship + /// attribute of a given entity. It ensures that the relationship value + /// that it returns is attached to the database without reattaching duplicates instances + /// to the change tracker. + /// + private object GetTrackedRelationshipValue(RelationshipAttribute relationshipAttr, TEntity entity, out bool wasAlreadyAttached) + { + wasAlreadyAttached = false; + if (relationshipAttr is HasOneAttribute hasOneAttribute) + { + /// This adds support for resource-entity separation in the case of one-to-one. + var relationshipValue = GetEntityResourceSeparationValue(entity, hasOneAttribute) ?? (IIdentifiable)hasOneAttribute.GetValue(entity); + if (relationshipValue == null) + return null; + return GetTrackedHasOneRelationshipValue(relationshipValue, hasOneAttribute, ref wasAlreadyAttached); + } + else + { + IEnumerable relationshipValueList = (IEnumerable)relationshipAttr.GetValue(entity); + /// This adds support for resource-entity separation in the case of one-to-many. + /// todo: currently there is no support for many to many relations. + if (relationshipAttr is HasManyAttribute hasMany) + relationshipValueList = GetEntityResourceSeparationValue(entity, hasMany) ?? relationshipValueList; + if (relationshipValueList == null) return null; + return GetTrackedManyRelationshipValue(relationshipValueList, relationshipAttr, ref wasAlreadyAttached); + } + } + + private IList GetTrackedManyRelationshipValue(IEnumerable relationshipValueList, RelationshipAttribute relationshipAttr, ref bool wasAlreadyAttached) + { + if (relationshipValueList == null) return null; + bool _wasAlreadyAttached = false; + var trackedPointerCollection = relationshipValueList.Select(pointer => + { + var tracked = AttachOrGetTracked(pointer); + if (tracked != null) _wasAlreadyAttached = true; + return Convert.ChangeType(tracked ?? pointer, relationshipAttr.Type); + }).ToList().Cast(relationshipAttr.Type); + if (_wasAlreadyAttached) wasAlreadyAttached = true; + return (IList)trackedPointerCollection; + } + + private IIdentifiable GetTrackedHasOneRelationshipValue(IIdentifiable relationshipValue, HasOneAttribute hasOneAttribute, ref bool wasAlreadyAttached) + { + + var tracked = AttachOrGetTracked(relationshipValue); + if (tracked != null) wasAlreadyAttached = true; + return tracked ?? relationshipValue; + } + + /// public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds) { // TODO: it would be better to let this be determined within the relationship attribute... @@ -343,61 +397,23 @@ public async Task> ToListAsync(IQueryable entiti : entities.ToList(); } + /// - /// This is used to allow creation of HasMany relationships when the - /// dependent side of the relationship already exists. + /// Before assigning new relationship values (UpdateAsync), we need to + /// attach the current relationship state to the dbcontext, else + /// it will not perform a complete-replace which is required for + /// one-to-many and many-to-many. /// - private void AttachHasManyAndHasManyThroughPointers(TEntity entity) + protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute relationshipAttribute) { - var relationships = _jsonApiContext.HasManyRelationshipPointers.Get(); - - foreach (var attribute in relationships.Keys.ToArray()) + if (relationshipAttribute is HasManyThroughAttribute throughAttribute) { - IEnumerable pointers; - if (attribute is HasManyThroughAttribute hasManyThrough) - { - pointers = relationships[attribute].Cast(); - } - else - { - pointers = GetRelationshipPointers(entity, (HasManyAttribute)attribute) ?? relationships[attribute].Cast(); - } + _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).Load(); - if (pointers == null) continue; - bool alreadyTracked = false; - Type entityType = null; - var newPointerCollection = pointers.Select(pointer => - { - entityType = pointer.GetType(); - var tracked = AttachOrGetTracked(pointer); - if (tracked != null) alreadyTracked = true; - return Convert.ChangeType(tracked ?? pointer, entityType); - }).ToList().Cast(entityType); - - if (alreadyTracked || pointers != relationships[attribute]) relationships[attribute] = (IList)newPointerCollection; } - } - - /// - /// Before assigning new relationship values (updateasync), we need to - /// attach load the current relationship state into the dbcontext, else - /// there won't be a complete-replace for one-to-many and many-to-many. - /// - /// Old entity. - protected void LoadCurrentRelationships(TEntity oldEntity) - { - foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) + else if (relationshipAttribute is HasManyAttribute hasManyAttribute) { - var relationshipValue = relationshipEntry.Value; - if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) - { - _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).Load(); - - } - else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) - { - _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).Load(); - } + _context.Entry(oldEntity).Collection(hasManyAttribute.InternalRelationshipName).Load(); } } @@ -407,25 +423,16 @@ protected void LoadCurrentRelationships(TEntity oldEntity) /// retrieve from the context WHICH relationships to update, but the actual /// values should not come from the context. /// - protected void AssignRelationshipValues(TEntity oldEntity) + protected void AssignRelationshipValue(TEntity oldEntity, object relationshipValue, RelationshipAttribute relationshipAttribute) { - foreach (var relationshipEntry in _jsonApiContext.RelationshipsToUpdate) + if (relationshipAttribute is HasManyThroughAttribute throughAttribute) { - var relationshipValue = relationshipEntry.Value; - if (relationshipEntry.Key is HasManyThroughAttribute throughAttribute) - { - AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); - } - else if (relationshipEntry.Key is HasManyAttribute hasManyAttribute) - { - // todo: need to load inverse relationship here, see issue #502 - hasManyAttribute.SetValue(oldEntity, relationshipValue); - } - else if (relationshipEntry.Key is HasOneAttribute hasOneAttribute) - { - // todo: need to load inverse relationship here, see issue #502 - hasOneAttribute.SetValue(oldEntity, relationshipValue); - } + // todo: this logic should be put in the HasManyThrough attribute + AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); + } + else + { + relationshipAttribute.SetValue(oldEntity, relationshipValue); } } @@ -451,33 +458,23 @@ private void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasMan } /// - /// This is used to allow creation of HasOne relationships when the + /// A helper method that gets the relationship value in the case of + /// entity resource separation. /// - private void AttachHasOnePointers(TEntity entity) - { - var relationships = _jsonApiContext - .HasOneRelationshipPointers - .Get(); - - foreach (var attribute in relationships.Keys.ToArray()) - { - var pointer = GetRelationshipPointer(entity, attribute) ?? relationships[attribute]; - if (pointer == null) return; - var tracked = AttachOrGetTracked(pointer); - if (tracked != null || pointer != relationships[attribute]) relationships[attribute] = tracked ?? pointer; - } - } - - IIdentifiable GetRelationshipPointer(TEntity principalEntity, HasOneAttribute attribute) + IIdentifiable GetEntityResourceSeparationValue(TEntity entity, HasOneAttribute attribute) { if (attribute.EntityPropertyName == null) { return null; } - return (IIdentifiable)principalEntity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(principalEntity); + return (IIdentifiable)entity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(entity); } - IEnumerable GetRelationshipPointers(TEntity entity, HasManyAttribute attribute) + /// + /// A helper method that gets the relationship value in the case of + /// entity resource separation. + /// + IEnumerable GetEntityResourceSeparationValue(TEntity entity, HasManyAttribute attribute) { if (attribute.EntityPropertyName == null) { @@ -486,23 +483,31 @@ IEnumerable GetRelationshipPointers(TEntity entity, HasManyAttrib return ((IEnumerable)(entity.GetType().GetProperty(attribute.EntityPropertyName)?.GetValue(entity))).Cast(); } - // useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified - IIdentifiable AttachOrGetTracked(IIdentifiable pointer) + /// + /// Given a iidentifiable relationshipvalue, verify if an entity of the underlying + /// type with the same ID is already attached to the dbContext, and if so, return it. + /// If not, attach the relationship value to the dbContext. + /// + /// useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified + /// + IIdentifiable AttachOrGetTracked(IIdentifiable relationshipValue) { - var trackedEntity = _context.GetTrackedEntity(pointer); + var trackedEntity = _context.GetTrackedEntity(relationshipValue); if (trackedEntity != null) { /// there already was an instance of this type and ID tracked - /// by EF Core. Reattaching will produce a conflict, and from now on we - /// will use the already attached one instead. (This entry might - /// contain updated fields as a result of business logic) + /// by EF Core. Reattaching will produce a conflict, so from now on we + /// will use the already attached instance instead. This entry might + /// contain updated fields as a result of business logic elsewhere in the application return trackedEntity; } /// the relationship pointer is new to EF Core, but we are sure - /// it exists in the database (json:api spec), so we attach it. - _context.Entry(pointer).State = EntityState.Unchanged; + /// it exists in the database, so we attach it. In this case, as per + /// the json:api spec, we can also safely assume that no fields of + /// this entity were updated. + _context.Entry(relationshipValue).State = EntityState.Unchanged; return null; } } From 7aea60ceeee6e731d4249700fc43bac95e259609 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 09:40:07 +0200 Subject: [PATCH 13/29] feat: decoupled IJsonApiContext from repository wrt updating resources --- .../Data/DefaultEntityRepository.cs | 48 ++++++++----------- .../Serialization/JsonApiDeSerializer.cs | 27 +++++++---- 2 files changed, 38 insertions(+), 37 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index be47d0488d..89bba5775b 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -171,44 +171,34 @@ public virtual async Task CreateAsync(TEntity entity) /// public void DetachRelationshipPointers(TEntity entity) { - foreach (var hasOneRelationship in _jsonApiContext.HasOneRelationshipPointers.Get()) - { - var hasOne = (HasOneAttribute)hasOneRelationship.Key; - if (hasOne.EntityPropertyName != null) - { - var relatedEntity = entity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(entity); - if (relatedEntity != null) - _context.Entry(relatedEntity).State = EntityState.Detached; - } - else - { - _context.Entry(hasOneRelationship.Value).State = EntityState.Detached; - } - } - foreach (var hasManyRelationship in _jsonApiContext.HasManyRelationshipPointers.Get()) + foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate.Keys) { - var hasMany = (HasManyAttribute)hasManyRelationship.Key; - if (hasMany.EntityPropertyName != null) + if (relationshipAttr is HasOneAttribute hasOneAttr) { - var relatedList = (IList)entity.GetType().GetProperty(hasMany.EntityPropertyName)?.GetValue(entity); - foreach (var related in relatedList) - { - _context.Entry(related).State = EntityState.Detached; - } + var relationshipValue = GetEntityResourceSeparationValue(entity, hasOneAttr) ?? (IIdentifiable)hasOneAttr.GetValue(entity); + if (relationshipValue == null) continue; + _context.Entry(relationshipValue).State = EntityState.Detached; + } - else + else { - foreach (var pointer in hasManyRelationship.Value) + IEnumerable relationshipValueList = (IEnumerable)relationshipAttr.GetValue(entity); + /// This adds support for resource-entity separation in the case of one-to-many. + /// todo: currently there is no support for many to many relations. + if (relationshipAttr is HasManyAttribute hasMany) + relationshipValueList = GetEntityResourceSeparationValue(entity, hasMany) ?? relationshipValueList; + if (relationshipValueList == null) continue; + foreach (var pointer in relationshipValueList) { _context.Entry(pointer).State = EntityState.Detached; } + /// detaching has many relationships is not sufficient to + /// trigger a full reload of relationships: the navigation + /// property actually needs to be nulled out, otherwise + /// EF will still add duplicate instances to the collection + relationshipAttr.SetValue(entity, null); } - - // 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 - hasManyRelationship.Key.SetValue(entity, null); } } diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index c47b81ee05..27090c5bb7 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -155,7 +155,11 @@ private object SetEntityAttributes( continue; var convertedValue = ConvertAttrValue(newValue, attr.PropertyInfo.PropertyType); attr.SetValue(entity, convertedValue); - _jsonApiContext.AttributesToUpdate[attr] = convertedValue; + /// todo: as a part of the process of decoupling JADNC (specifically + /// through the decoupling IJsonApiContext), we now no longer need to + /// store the updated relationship values in this property. For now + /// just assigning null as value, will remove this property later as a whole. + _jsonApiContext.AttributesToUpdate[attr] = null; } } @@ -249,7 +253,10 @@ private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, throw new JsonApiException(400, $"Cannot set required relationship identifier '{hasOneAttr.IdentifiablePropertyName}' to null because it is a non-nullable type."); var convertedValue = TypeHelper.ConvertType(foreignKeyPropertyValue, foreignKeyProperty.PropertyType); - foreignKeyProperty.SetValue(entity, convertedValue); + /// todo: as a part of the process of decoupling JADNC (specifically + /// through the decoupling IJsonApiContext), we now no longer need to + /// store the updated relationship values in this property. For now + /// just assigning null as value, will remove this property later as a whole. if (convertedValue == null ) _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); } } @@ -270,10 +277,11 @@ private void SetHasOneNavigationPropertyValue(object entity, HasOneAttribute has if (includedRelationshipObject != null) hasOneAttr.SetValue(entity, includedRelationshipObject); - // we need to store the fact that this relationship was included in the payload - // for EF, the repository will use these pointers to make ensure we don't try to - // create resources if they already exist, we just need to create the relationship - _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, includedRelationshipObject); + /// todo: as a part of the process of decoupling JADNC (specifically + /// through the decoupling IJsonApiContext), we now no longer need to + /// store the updated relationship values in this property. For now + /// just assigning null as value, will remove this property later as a whole. + _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); } } @@ -300,8 +308,11 @@ private object SetHasManyRelationship(object entity, var convertedCollection = TypeHelper.ConvertCollection(relatedResources, attr.Type); attr.SetValue(entity, convertedCollection); - //_jsonApiContext.RelationshipsToUpdate[attr] = convertedCollection; // WHAT exactly is the use of these two different collections..? - _jsonApiContext.HasManyRelationshipPointers.Add(attr, convertedCollection); + /// todo: as a part of the process of decoupling JADNC (specifically + /// through the decoupling IJsonApiContext), we now no longer need to + /// store the updated relationship values in this property. For now + /// just assigning null as value, will remove this property later as a whole. + _jsonApiContext.HasManyRelationshipPointers.Add(attr, null); } return entity; From 652d65f0841dded77a700fe460925cbc0c3a9836 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 10:00:52 +0200 Subject: [PATCH 14/29] fix: resource separation issue| --- .../Data/DefaultEntityRepository.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 89bba5775b..3e0a64ea1b 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -257,20 +257,28 @@ private object GetTrackedRelationshipValue(RelationshipAttribute relationshipAtt } } + // helper method used in GetTrackedRelationshipValue. See comments there. private IList GetTrackedManyRelationshipValue(IEnumerable relationshipValueList, RelationshipAttribute relationshipAttr, ref bool wasAlreadyAttached) { if (relationshipValueList == null) return null; bool _wasAlreadyAttached = false; + Type entityType = null; var trackedPointerCollection = relationshipValueList.Select(pointer => { + /// todo: we can't just use relationshipAttr.Type because + /// this will point to the Resource type in the case of entity resource + /// separation. We should consider to store entity type on + /// the relationship attribute too. + entityType = entityType ?? pointer.GetType(); var tracked = AttachOrGetTracked(pointer); if (tracked != null) _wasAlreadyAttached = true; - return Convert.ChangeType(tracked ?? pointer, relationshipAttr.Type); - }).ToList().Cast(relationshipAttr.Type); + return Convert.ChangeType(tracked ?? pointer, entityType); + }).ToList().Cast(entityType); if (_wasAlreadyAttached) wasAlreadyAttached = true; return (IList)trackedPointerCollection; } + // helper method used in GetTrackedRelationshipValue. See comments there. private IIdentifiable GetTrackedHasOneRelationshipValue(IIdentifiable relationshipValue, HasOneAttribute hasOneAttribute, ref bool wasAlreadyAttached) { From 9838627849793e828f969d56a761ae58f66f23a8 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 10:12:02 +0200 Subject: [PATCH 15/29] chore: cherry picked inverse relationships from hooks branch --- .../Data/DefaultEntityRepository.cs | 1 - .../IApplicationBuilderExtensions.cs | 7 ++ .../IServiceCollectionExtensions.cs | 3 + .../Internal/InverseRelationships.cs | 65 +++++++++++++++++++ .../Models/HasManyAttribute.cs | 6 +- .../Models/HasOneAttribute.cs | 5 +- .../Models/RelationshipAttribute.cs | 6 +- 7 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 src/JsonApiDotNetCore/Internal/InverseRelationships.cs diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 3e0a64ea1b..0e1757f774 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -281,7 +281,6 @@ private IList GetTrackedManyRelationshipValue(IEnumerable relatio // helper method used in GetTrackedRelationshipValue. See comments there. private IIdentifiable GetTrackedHasOneRelationshipValue(IIdentifiable relationshipValue, HasOneAttribute hasOneAttribute, ref bool wasAlreadyAttached) { - var tracked = AttachOrGetTracked(relationshipValue); if (tracked != null) wasAlreadyAttached = true; return tracked ?? relationshipValue; diff --git a/src/JsonApiDotNetCore/Extensions/IApplicationBuilderExtensions.cs b/src/JsonApiDotNetCore/Extensions/IApplicationBuilderExtensions.cs index 20cee086d6..e66b51a7a0 100644 --- a/src/JsonApiDotNetCore/Extensions/IApplicationBuilderExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/IApplicationBuilderExtensions.cs @@ -4,6 +4,7 @@ using JsonApiDotNetCore.Middleware; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; namespace JsonApiDotNetCore.Extensions @@ -21,6 +22,12 @@ public static IApplicationBuilder UseJsonApi(this IApplicationBuilder app, bool if (useMvc) app.UseMvc(); + using (var scope = app.ApplicationServices.CreateScope()) + { + var inverseRelationshipResolver = scope.ServiceProvider.GetService(); + inverseRelationshipResolver?.Resolve(); + } + return app; } diff --git a/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs b/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs index d1c1de352c..a44020ae5b 100644 --- a/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs @@ -155,6 +155,9 @@ public static void AddJsonApiInternals( services.AddScoped(); services.AddScoped(); + services.AddScoped(); + + // services.AddScoped(); } diff --git a/src/JsonApiDotNetCore/Internal/InverseRelationships.cs b/src/JsonApiDotNetCore/Internal/InverseRelationships.cs new file mode 100644 index 0000000000..02ad493910 --- /dev/null +++ b/src/JsonApiDotNetCore/Internal/InverseRelationships.cs @@ -0,0 +1,65 @@ +using System; +using JsonApiDotNetCore.Data; +using JsonApiDotNetCore.Models; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Metadata; + +namespace JsonApiDotNetCore.Internal +{ + /// + /// Responsible for populating the RelationshipAttribute InverseNavigation property. + /// + /// This service is instantiated in the configure phase of the application. + /// + /// When using a data access layer different from EF Core, and when using ResourceHooks + /// that depend on the inverse navigation property (BeforeImplicitUpdateRelationship), + /// you will need to override this service, or pass along the inverseNavigationProperty in + /// the RelationshipAttribute. + /// + public interface IInverseRelationships + { + void Resolve(); + } + + public class InverseRelationships : IInverseRelationships + { + private readonly ResourceGraph _graph; + private readonly IDbContextResolver _resolver; + + public InverseRelationships(IResourceGraph graph, IDbContextResolver resolver = null) + { + _graph = (ResourceGraph)graph; + _resolver = resolver; + } + + public void Resolve() + { + if (EntityFrameworkCoreIsEnabled()) + { + DbContext context = _resolver.GetContext(); + + foreach (ContextEntity ce in _graph.Entities) + { + IEntityType meta = context.Model.FindEntityType(ce.EntityType); + if (meta == null) continue; + foreach (var attr in ce.Relationships) + { + if (attr is HasManyThroughAttribute) continue; + INavigation inverseNavigation = meta.FindNavigation(attr.InternalRelationshipName)?.FindInverse(); + attr.InverseNavigation = inverseNavigation?.Name; + } + } + } + } + + /// + /// If EF Core is not being used, we're expecting the resolver to not be registered. + /// + /// true, if entity framework core was enabled, false otherwise. + /// Resolver. + private bool EntityFrameworkCoreIsEnabled() + { + return _resolver != null; + } + } +} diff --git a/src/JsonApiDotNetCore/Models/HasManyAttribute.cs b/src/JsonApiDotNetCore/Models/HasManyAttribute.cs index 2d69b0e9c7..37fe42c9af 100644 --- a/src/JsonApiDotNetCore/Models/HasManyAttribute.cs +++ b/src/JsonApiDotNetCore/Models/HasManyAttribute.cs @@ -24,9 +24,11 @@ public class HasManyAttribute : RelationshipAttribute /// /// /// - public HasManyAttribute(string publicName = null, Link documentLinks = Link.All, bool canInclude = true, string mappedBy = null) + public HasManyAttribute(string publicName = null, Link documentLinks = Link.All, bool canInclude = true, string mappedBy = null, string inverseNavigationProperty = null) : base(publicName, documentLinks, canInclude, mappedBy) - { } + { + InverseNavigation = inverseNavigationProperty; + } /// /// Sets the value of the property identified by this attribute diff --git a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs index adca016c43..efa34bcf8b 100644 --- a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs +++ b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs @@ -28,11 +28,14 @@ public class HasOneAttribute : RelationshipAttribute /// /// /// - public HasOneAttribute(string publicName = null, Link documentLinks = Link.All, bool canInclude = true, string withForeignKey = null, string mappedBy = null) + public HasOneAttribute(string publicName = null, Link documentLinks = Link.All, bool canInclude = true, string withForeignKey = null, string mappedBy = null, string inverseNavigationProperty = null) : base(publicName, documentLinks, canInclude, mappedBy) { _explicitIdentifiablePropertyName = withForeignKey; + InverseNavigation = inverseNavigationProperty; } + public string InverseNavigation { get; internal set; } + private readonly string _explicitIdentifiablePropertyName; diff --git a/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs b/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs index bc6982f062..5df2140cd7 100644 --- a/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs +++ b/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs @@ -15,8 +15,10 @@ protected RelationshipAttribute(string publicName, Link documentLinks, bool canI } public string PublicRelationshipName { get; internal set; } - public string InternalRelationshipName { get; internal set; } - + public string InternalRelationshipName { get; internal set; } + + public string InverseNavigation { get; internal set; } + /// /// The related entity type. This does not necessarily match the navigation property type. /// In the case of a HasMany relationship, this value will be the generic argument type. From fbe69fcd951f93dad3a9835fae2eb148b922ac93 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 10:34:48 +0200 Subject: [PATCH 16/29] fix: tests --- .../Acceptance/Extensibility/RepositoryOverrideTests.cs | 2 +- .../Acceptance/TodoItemsControllerTests.cs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/RepositoryOverrideTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/RepositoryOverrideTests.cs index b161b14c87..d63575e263 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/RepositoryOverrideTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/RepositoryOverrideTests.cs @@ -59,7 +59,7 @@ public async Task Total_Record_Count_Included() // assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); foreach(var item in deserializedBody) - Assert.Equal(person.Id, item.OwnerId); + Assert.Equal(person.Id, item.Owner.Id); } [Fact] diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs index d28a6bd918..cb9b52f9b2 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs @@ -114,7 +114,7 @@ public async Task Can_Filter_By_Relationship_Id() // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.NotEmpty(deserializedBody); - Assert.Contains(deserializedBody, (i) => i.OwnerId == person.Id); + Assert.Contains(deserializedBody, (i) => i.Owner.Id == person.Id); } [Fact] @@ -435,7 +435,8 @@ public async Task Can_Get_TodoItem_WithOwner() // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); var deserializedBody = (TodoItem)_fixture.GetService().Deserialize(body); - Assert.Equal(person.Id, deserializedBody.OwnerId); + + Assert.Equal(person.Id, deserializedBody.Owner.Id); Assert.Equal(todoItem.Id, deserializedBody.Id); Assert.Equal(todoItem.Description, deserializedBody.Description); Assert.Equal(todoItem.Ordinal, deserializedBody.Ordinal); From 35a2f54731184e18b7b5e3da7a544c9fcd545655 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 10:51:18 +0200 Subject: [PATCH 17/29] feat: implicit remove support --- .../Data/DefaultEntityRepository.cs | 47 ++++++++++++++----- .../Models/HasOneAttribute.cs | 2 - .../Spec/UpdatingRelationshipsTests.cs | 4 +- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 0e1757f774..89dc57608b 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -147,14 +147,15 @@ public virtual async Task CreateAsync(TEntity entity) foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate?.Keys) { var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, entity, out bool wasAlreadyTracked); - // LoadInverseRelationships(trackedRelationshipValue, relationshipAttribute) + LoadInverseRelationships(trackedRelationshipValue, relationshipAttr); if (wasAlreadyTracked) { /// We only need to reassign the relationship value to the to-be-added /// entity when we're using a different instance (because this different one /// was already tracked) than the one assigned to the to-be-created entity. AssignRelationshipValue(entity, trackedRelationshipValue, relationshipAttr); - } else if (relationshipAttr is HasManyThroughAttribute throughAttr) + } + else if (relationshipAttr is HasManyThroughAttribute throughAttr) { /// even if we don't have to reassign anything because of already tracked /// entities, we still need to assign the "through" entities in the case of many-to-many. @@ -167,6 +168,26 @@ public virtual async Task CreateAsync(TEntity entity) return entity; } + /// + /// Loads the inverse relationships to prevent foreign key constraints from being violated + /// to support implicit removes, see https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/502. + /// + private void LoadInverseRelationships(object trackedRelationshipValue, RelationshipAttribute relationshipAttr) + { + if (relationshipAttr.InverseNavigation == null) return; + if (relationshipAttr is HasOneAttribute hasOneAttr) + { + _context.Entry((IIdentifiable)trackedRelationshipValue).Reference(hasOneAttr.InverseNavigation).Load(); + } + else if (relationshipAttr is HasManyAttribute hasManyAttr) + { + foreach (IIdentifiable relationshipValue in (IList)trackedRelationshipValue) + { + _context.Entry((IIdentifiable)trackedRelationshipValue).Reference(hasManyAttr.InverseNavigation).Load(); + } + } + } + /// public void DetachRelationshipPointers(TEntity entity) @@ -174,14 +195,14 @@ public void DetachRelationshipPointers(TEntity entity) foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate.Keys) { - if (relationshipAttr is HasOneAttribute hasOneAttr) + if (relationshipAttr is HasOneAttribute hasOneAttr) { var relationshipValue = GetEntityResourceSeparationValue(entity, hasOneAttr) ?? (IIdentifiable)hasOneAttr.GetValue(entity); if (relationshipValue == null) continue; _context.Entry(relationshipValue).State = EntityState.Detached; } - else + else { IEnumerable relationshipValueList = (IEnumerable)relationshipAttr.GetValue(entity); /// This adds support for resource-entity separation in the case of one-to-many. @@ -220,7 +241,7 @@ public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) { LoadCurrentRelationships(oldEntity, relationshipAttr); var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out bool wasAlreadyTracked); - // LoadInverseRelationships(trackedRelationshipValue, relationshipAttribute) + LoadInverseRelationships(trackedRelationshipValue, relationshipAttr); AssignRelationshipValue(oldEntity, trackedRelationshipValue, relationshipAttr); } await _context.SaveChangesAsync(); @@ -237,21 +258,21 @@ public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) private object GetTrackedRelationshipValue(RelationshipAttribute relationshipAttr, TEntity entity, out bool wasAlreadyAttached) { wasAlreadyAttached = false; - if (relationshipAttr is HasOneAttribute hasOneAttribute) + if (relationshipAttr is HasOneAttribute hasOneAttr) { /// This adds support for resource-entity separation in the case of one-to-one. - var relationshipValue = GetEntityResourceSeparationValue(entity, hasOneAttribute) ?? (IIdentifiable)hasOneAttribute.GetValue(entity); - if (relationshipValue == null) - return null; - return GetTrackedHasOneRelationshipValue(relationshipValue, hasOneAttribute, ref wasAlreadyAttached); + var relationshipValue = GetEntityResourceSeparationValue(entity, hasOneAttr) ?? (IIdentifiable)hasOneAttr.GetValue(entity); + if (relationshipValue == null) + return null; + return GetTrackedHasOneRelationshipValue(relationshipValue, hasOneAttr, ref wasAlreadyAttached); } else { IEnumerable relationshipValueList = (IEnumerable)relationshipAttr.GetValue(entity); /// This adds support for resource-entity separation in the case of one-to-many. /// todo: currently there is no support for many to many relations. - if (relationshipAttr is HasManyAttribute hasMany) - relationshipValueList = GetEntityResourceSeparationValue(entity, hasMany) ?? relationshipValueList; + if (relationshipAttr is HasManyAttribute hasMany) + relationshipValueList = GetEntityResourceSeparationValue(entity, hasMany) ?? relationshipValueList; if (relationshipValueList == null) return null; return GetTrackedManyRelationshipValue(relationshipValueList, relationshipAttr, ref wasAlreadyAttached); } @@ -279,7 +300,7 @@ private IList GetTrackedManyRelationshipValue(IEnumerable relatio } // helper method used in GetTrackedRelationshipValue. See comments there. - private IIdentifiable GetTrackedHasOneRelationshipValue(IIdentifiable relationshipValue, HasOneAttribute hasOneAttribute, ref bool wasAlreadyAttached) + private IIdentifiable GetTrackedHasOneRelationshipValue(IIdentifiable relationshipValue, HasOneAttribute hasOneAttr, ref bool wasAlreadyAttached) { var tracked = AttachOrGetTracked(relationshipValue); if (tracked != null) wasAlreadyAttached = true; diff --git a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs index efa34bcf8b..d46d5adfdf 100644 --- a/src/JsonApiDotNetCore/Models/HasOneAttribute.cs +++ b/src/JsonApiDotNetCore/Models/HasOneAttribute.cs @@ -34,8 +34,6 @@ public HasOneAttribute(string publicName = null, Link documentLinks = Link.All, _explicitIdentifiablePropertyName = withForeignKey; InverseNavigation = inverseNavigationProperty; } - public string InverseNavigation { get; internal set; } - private readonly string _explicitIdentifiablePropertyName; diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 4e77d155b5..f6373e18f9 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -664,8 +664,8 @@ public async Task Updating_ToOne_Relationship_With_Implicit_Remove() // Assert var body = await response.Content.ReadAsStringAsync(); - // this test currently fails, will be adressed in next PR - //Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + + Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); } } From 6e6f7fa66117b4dd7185f9cdd1c1a4bdca6cb775 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 11:02:50 +0200 Subject: [PATCH 18/29] fix: test --- .../ResourceEntitySeparationExampleTests/Acceptance/AddTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ResourceEntitySeparationExampleTests/Acceptance/AddTests.cs b/test/ResourceEntitySeparationExampleTests/Acceptance/AddTests.cs index f50ad734b7..53105ee4b6 100644 --- a/test/ResourceEntitySeparationExampleTests/Acceptance/AddTests.cs +++ b/test/ResourceEntitySeparationExampleTests/Acceptance/AddTests.cs @@ -94,7 +94,7 @@ public async Task Can_Create_Course_With_Department_Id() Assert.Equal(course.Number, data.Number); Assert.Equal(course.Title, data.Title); Assert.Equal(course.Description, data.Description); - Assert.Equal(department.Id, data.DepartmentId); + Assert.Equal(department.Id, data.Department.Id); } [Fact] From c1d472d5cac36a11a4ca0c3680e6d799dfdd5073 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 12:41:10 +0200 Subject: [PATCH 19/29] fix: bugs with inverse relationship loading --- .../Data/DefaultEntityRepository.cs | 22 ++++-- .../Spec/UpdatingRelationshipsTests.cs | 69 ++++++++++++++++++- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 89dc57608b..5ccdf0ac72 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -174,20 +174,34 @@ public virtual async Task CreateAsync(TEntity entity) /// private void LoadInverseRelationships(object trackedRelationshipValue, RelationshipAttribute relationshipAttr) { - if (relationshipAttr.InverseNavigation == null) return; + if (relationshipAttr.InverseNavigation == null || trackedRelationshipValue == null) return; if (relationshipAttr is HasOneAttribute hasOneAttr) { - _context.Entry((IIdentifiable)trackedRelationshipValue).Reference(hasOneAttr.InverseNavigation).Load(); + var relationEntry = _context.Entry((IIdentifiable)trackedRelationshipValue); + if (IsHasOneRelationship(hasOneAttr.InverseNavigation, trackedRelationshipValue.GetType())) + { + relationEntry.Reference(hasOneAttr.InverseNavigation).Load(); + } else + { + relationEntry.Collection(hasOneAttr.InverseNavigation).Load(); + } } - else if (relationshipAttr is HasManyAttribute hasManyAttr) + else if (relationshipAttr is HasManyAttribute hasManyAttr && !(relationshipAttr is HasManyThroughAttribute)) { foreach (IIdentifiable relationshipValue in (IList)trackedRelationshipValue) { - _context.Entry((IIdentifiable)trackedRelationshipValue).Reference(hasManyAttr.InverseNavigation).Load(); + _context.Entry(relationshipValue).Reference(hasManyAttr.InverseNavigation).Load(); } } } + private bool IsHasOneRelationship(string inverseNavigation, Type type) + { + var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.Single(r => r.InternalRelationshipName == inverseNavigation); + if (relationshipAttr is HasOneAttribute) return true; + return false; + } + /// public void DetachRelationshipPointers(TEntity entity) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index f6373e18f9..6ba9c8ba6c 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -633,7 +633,7 @@ public async Task Updating_ToOne_Relationship_With_Implicit_Remove() var person2 = _personFaker.Generate(); context.People.AddRange(new List() { person1, person2 }); await context.SaveChangesAsync(); - + var passportId = person1.PassportId; var content = new { data = new @@ -644,7 +644,7 @@ public async Task Updating_ToOne_Relationship_With_Implicit_Remove() { { "passport", new { - data = new { type = "passports", id = $"{person1.PassportId}" } + data = new { type = "passports", id = $"{passportId}" } } } } @@ -661,12 +661,75 @@ public async Task Updating_ToOne_Relationship_With_Implicit_Remove() // Act var response = await _fixture.Client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); // Assert + + Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + var dbPerson = context.People.AsNoTracking().Where(p => p.Id == person2.Id).Include("Passport").FirstOrDefault(); + Assert.Equal(passportId, dbPerson.Passport.Id); + } + + [Fact] + public async Task Updating_ToMany_Relationship_With_Implicit_Remove() + { + // Arrange + var context = _fixture.GetService(); + var person1 = _personFaker.Generate(); + person1.TodoItems = _todoItemFaker.Generate(3).ToList(); + var person2 = _personFaker.Generate(); + person2.TodoItems = _todoItemFaker.Generate(2).ToList(); + context.People.AddRange(new List() { person1, person2 }); + await context.SaveChangesAsync(); + var todoItem1Id = person1.TodoItems[0].Id; + var todoItem2Id = person1.TodoItems[1].Id; + + var content = new + { + data = new + { + type = "people", + id = person2.Id, + relationships = new Dictionary + { + { "todo-items", new + { + data = new List + { + new { + type = "todo-items", + id = $"{todoItem1Id}" + }, + new { + type = "todo-items", + id = $"{todoItem2Id}" + } + } + } + } + } + } + }; + + var httpMethod = new HttpMethod("PATCH"); + var route = $"/api/v1/people/{person2.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 _fixture.Client.SendAsync(request); var body = await response.Content.ReadAsStringAsync(); - Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + // Assert + Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + var dbPerson = context.People.AsNoTracking().Where(p => p.Id == person2.Id).Include("TodoItems").FirstOrDefault(); + Assert.Equal(2, dbPerson.TodoItems.Count); + Assert.NotNull(dbPerson.TodoItems.SingleOrDefault(ti => ti.Id == todoItem1Id)); + Assert.NotNull(dbPerson.TodoItems.SingleOrDefault(ti => ti.Id == todoItem2Id)); } } } From f45972f2bff41f118bad96b3dadc69caf656b883 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 12:59:04 +0200 Subject: [PATCH 20/29] tests: implicit remove through create tests --- .../Data/DefaultEntityRepository.cs | 13 +- .../Acceptance/Spec/CreatingDataTests.cs | 114 ++++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 5ccdf0ac72..2b5bf900d1 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -195,9 +195,10 @@ private void LoadInverseRelationships(object trackedRelationshipValue, Relations } } - private bool IsHasOneRelationship(string inverseNavigation, Type type) + + private bool IsHasOneRelationship(string internalRelationshipName, Type type) { - var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.Single(r => r.InternalRelationshipName == inverseNavigation); + var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.Single(r => r.InternalRelationshipName == internalRelationshipName); if (relationshipAttr is HasOneAttribute) return true; return false; } @@ -455,7 +456,7 @@ protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute /// retrieve from the context WHICH relationships to update, but the actual /// values should not come from the context. /// - protected void AssignRelationshipValue(TEntity oldEntity, object relationshipValue, RelationshipAttribute relationshipAttribute) + private void AssignRelationshipValue(TEntity oldEntity, object relationshipValue, RelationshipAttribute relationshipAttribute) { if (relationshipAttribute is HasManyThroughAttribute throughAttribute) { @@ -493,7 +494,7 @@ private void AssignHasManyThrough(TEntity entity, HasManyThroughAttribute hasMan /// A helper method that gets the relationship value in the case of /// entity resource separation. /// - IIdentifiable GetEntityResourceSeparationValue(TEntity entity, HasOneAttribute attribute) + private IIdentifiable GetEntityResourceSeparationValue(TEntity entity, HasOneAttribute attribute) { if (attribute.EntityPropertyName == null) { @@ -506,7 +507,7 @@ IIdentifiable GetEntityResourceSeparationValue(TEntity entity, HasOneAttribute a /// A helper method that gets the relationship value in the case of /// entity resource separation. /// - IEnumerable GetEntityResourceSeparationValue(TEntity entity, HasManyAttribute attribute) + private IEnumerable GetEntityResourceSeparationValue(TEntity entity, HasManyAttribute attribute) { if (attribute.EntityPropertyName == null) { @@ -522,7 +523,7 @@ IEnumerable GetEntityResourceSeparationValue(TEntity entity, HasM /// /// useful article: https://stackoverflow.com/questions/30987806/dbset-attachentity-vs-dbcontext-entryentity-state-entitystate-modified /// - IIdentifiable AttachOrGetTracked(IIdentifiable relationshipValue) + private IIdentifiable AttachOrGetTracked(IIdentifiable relationshipValue) { var trackedEntity = _context.GetTrackedEntity(relationshipValue); diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/CreatingDataTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/CreatingDataTests.cs index a34312ca2f..93137b4ee2 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/CreatingDataTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/CreatingDataTests.cs @@ -17,6 +17,7 @@ using Microsoft.EntityFrameworkCore; using Newtonsoft.Json; using Xunit; +using Person = JsonApiDotNetCoreExample.Models.Person; namespace JsonApiDotNetCoreExampleTests.Acceptance.Spec { @@ -26,6 +27,7 @@ public class CreatingDataTests private TestFixture _fixture; private IJsonApiContext _jsonApiContext; private Faker _todoItemFaker; + private Faker _personFaker; public CreatingDataTests(TestFixture fixture) { @@ -35,6 +37,10 @@ public CreatingDataTests(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()); + _personFaker = new Faker() + .RuleFor(t => t.FirstName, f => f.Name.FirstName()) + .RuleFor(t => t.LastName, f => f.Name.LastName()); + } [Fact] @@ -586,5 +592,113 @@ public async Task Respond_409_ToIncorrectEntityType() // assert Assert.Equal(HttpStatusCode.Conflict, response.StatusCode); } + + [Fact] + public async Task Create_With_ToOne_Relationship_With_Implicit_Remove() + { + // Arrange + var context = _fixture.GetService(); + var passport = new Passport(); + var person1 = _personFaker.Generate(); + person1.Passport = passport; + context.People.AddRange(new List() { person1 }); + await context.SaveChangesAsync(); + var passportId = person1.PassportId; + var content = new + { + data = new + { + type = "people", + attributes = new Dictionary() { { "first-name", "Joe" } }, + relationships = new Dictionary + { + { "passport", new + { + data = new { type = "passports", id = $"{passportId}" } + } + } + } + } + }; + + var httpMethod = new HttpMethod("POST"); + var route = $"/api/v1/people"; + 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 _fixture.Client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + var personResult = _fixture.GetService().Deserialize(body); + + // Assert + + Assert.True(HttpStatusCode.Created == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + var dbPerson = context.People.AsNoTracking().Where(p => p.Id == personResult.Id).Include("Passport").FirstOrDefault(); + Assert.Equal(passportId, dbPerson.Passport.Id); + } + + [Fact] + public async Task Create_With_ToMany_Relationship_With_Implicit_Remove() + { + // Arrange + var context = _fixture.GetService(); + var person1 = _personFaker.Generate(); + person1.TodoItems = _todoItemFaker.Generate(3).ToList(); + context.People.AddRange(new List() { person1 }); + await context.SaveChangesAsync(); + var todoItem1Id = person1.TodoItems[0].Id; + var todoItem2Id = person1.TodoItems[1].Id; + + var content = new + { + data = new + { + type = "people", + attributes = new Dictionary() { { "first-name", "Joe" } }, + relationships = new Dictionary + { + { "todo-items", new + { + data = new List + { + new { + type = "todo-items", + id = $"{todoItem1Id}" + }, + new { + type = "todo-items", + id = $"{todoItem2Id}" + } + } + } + } + } + } + }; + + var httpMethod = new HttpMethod("POST"); + var route = $"/api/v1/people"; + 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 _fixture.Client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + var personResult = _fixture.GetService().Deserialize(body); + + // Assert + Assert.True(HttpStatusCode.Created == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}"); + var dbPerson = context.People.AsNoTracking().Where(p => p.Id == personResult.Id).Include("TodoItems").FirstOrDefault(); + Assert.Equal(2, dbPerson.TodoItems.Count); + Assert.NotNull(dbPerson.TodoItems.SingleOrDefault(ti => ti.Id == todoItem1Id)); + Assert.NotNull(dbPerson.TodoItems.SingleOrDefault(ti => ti.Id == todoItem2Id)); + } } } From d8b42177bb56250c4ff9c68760a1ef7ac4b5d9a3 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 13:10:34 +0200 Subject: [PATCH 21/29] feat: mark obsolete UpdateAsync(TId id, TEntity entity) method, add new one without id parameter --- .../Data/DefaultEntityRepository.cs | 13 ++++++++----- .../Data/IEntityWriteRepository.cs | 4 ++++ .../Services/EntityResourceService.cs | 4 +--- .../UnitTests/Data/DefaultEntityRepository_Tests.cs | 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 2b5bf900d1..da98748dd0 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -238,14 +238,16 @@ public void DetachRelationshipPointers(TEntity entity) } } - /// + [Obsolete("Use overload UpdateAsync(TEntity updatedEntity): providing parameter ID does no longer add anything relevant")] public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) { - /// WHY is parameter "entity" even passed along to this method?? - /// It does nothing! - - var oldEntity = await GetAsync(id); + return await UpdateAsync(updatedEntity); + } + /// + public virtual async Task UpdateAsync(TEntity updatedEntity) + { + var oldEntity = await GetAsync(updatedEntity.Id); if (oldEntity == null) return null; @@ -259,6 +261,7 @@ public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) LoadInverseRelationships(trackedRelationshipValue, relationshipAttr); AssignRelationshipValue(oldEntity, trackedRelationshipValue, relationshipAttr); } + await _context.SaveChangesAsync(); return oldEntity; } diff --git a/src/JsonApiDotNetCore/Data/IEntityWriteRepository.cs b/src/JsonApiDotNetCore/Data/IEntityWriteRepository.cs index 37c60b8521..aa143f53e4 100644 --- a/src/JsonApiDotNetCore/Data/IEntityWriteRepository.cs +++ b/src/JsonApiDotNetCore/Data/IEntityWriteRepository.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Threading.Tasks; using JsonApiDotNetCore.Models; @@ -14,6 +15,9 @@ public interface IEntityWriteRepository { Task CreateAsync(TEntity entity); + Task UpdateAsync(TEntity entity); + + [Obsolete("Use overload UpdateAsync(TEntity updatedEntity): providing parameter ID does no longer add anything relevant")] Task UpdateAsync(TId id, TEntity entity); Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds); diff --git a/src/JsonApiDotNetCore/Services/EntityResourceService.cs b/src/JsonApiDotNetCore/Services/EntityResourceService.cs index 3da420845c..b73866c4ed 100644 --- a/src/JsonApiDotNetCore/Services/EntityResourceService.cs +++ b/src/JsonApiDotNetCore/Services/EntityResourceService.cs @@ -154,9 +154,7 @@ public virtual async Task UpdateAsync(TId id, TResource resource) { var entity = MapIn(resource); - - - entity = await _entities.UpdateAsync(id, entity); + entity = await _entities.UpdateAsync(entity); return MapOut(entity); } diff --git a/test/UnitTests/Data/DefaultEntityRepository_Tests.cs b/test/UnitTests/Data/DefaultEntityRepository_Tests.cs index f5a77789e2..958639575e 100644 --- a/test/UnitTests/Data/DefaultEntityRepository_Tests.cs +++ b/test/UnitTests/Data/DefaultEntityRepository_Tests.cs @@ -60,14 +60,14 @@ public async Task UpdateAsync_Updates_Attributes_In_AttributesToUpdate() { { descAttr, - todoItemUpdates.Description + null //todoItemUpdates.Description } }; var repository = GetRepository(); // act - var updatedItem = await repository.UpdateAsync(_todoItem.Id, todoItemUpdates); + var updatedItem = await repository.UpdateAsync(todoItemUpdates); // assert Assert.NotNull(updatedItem); From 30765c3973f38d057b214ea43eb6946ef7515396 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 14:45:41 +0200 Subject: [PATCH 22/29] fix: #520 --- .../Formatters/JsonApiReader.cs | 55 ++++++++++++++++++- .../CamelCasedModelsControllerTests.cs | 1 + .../Acceptance/Spec/UpdatingDataTests.cs | 43 ++++++++++++++- .../Spec/UpdatingRelationshipsTests.cs | 1 + .../Acceptance/TodoItemsControllerTests.cs | 3 + 5 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs b/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs index b5c39ef4d5..15ce633cdf 100644 --- a/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs +++ b/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs @@ -1,7 +1,9 @@ using System; +using System.Collections; using System.IO; using System.Threading.Tasks; using JsonApiDotNetCore.Internal; +using JsonApiDotNetCore.Models; using JsonApiDotNetCore.Serialization; using JsonApiDotNetCore.Services; using Microsoft.AspNetCore.Mvc.Formatters; @@ -37,7 +39,7 @@ public Task ReadAsync(InputFormatterContext context) { var body = GetRequestBody(context.HttpContext.Request.Body); - object model =null; + object model = null; if (_jsonApiContext.IsRelationshipPath) { @@ -48,10 +50,29 @@ public Task ReadAsync(InputFormatterContext context) model = _deSerializer.Deserialize(body); } + if (model == null) { _logger?.LogError("An error occurred while de-serializing the payload"); } + + if (context.HttpContext.Request.Method == "PATCH") + { + bool idMissing; + if (model is IList list) + { + idMissing = CheckForId(list); + } + else + { + idMissing = CheckForId(model); + } + if (idMissing) + { + _logger?.LogError("Payload must include id attribute"); + throw new JsonApiException(400, "Payload must include id attribute"); + } + } return InputFormatterResult.SuccessAsync(model); } catch (Exception ex) @@ -62,6 +83,38 @@ public Task ReadAsync(InputFormatterContext context) } } + private bool CheckForId(object model) + { + if (model == null) return false; + if (model is ResourceObject ro) + { + if (string.IsNullOrEmpty(ro.Id)) return true; + } + else if (model is IIdentifiable identifiable) + { + if (string.IsNullOrEmpty(identifiable.StringId)) return true; + } + return false; + } + + private bool CheckForId(IList modelList) + { + foreach (var model in modelList) + { + if (model == null) continue; + if (model is ResourceObject ro) + { + if (string.IsNullOrEmpty(ro.Id)) return true; + } + else if (model is IIdentifiable identifiable) + { + if (string.IsNullOrEmpty(identifiable.StringId)) return true; + } + } + return false; + + } + private string GetRequestBody(Stream body) { using (var reader = new StreamReader(body)) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/CamelCasedModelsControllerTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/CamelCasedModelsControllerTests.cs index 533c59839a..b2059c10c9 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/CamelCasedModelsControllerTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/CamelCasedModelsControllerTests.cs @@ -143,6 +143,7 @@ public async Task Can_Patch_CamelCasedModels() data = new { type = "camelCasedModels", + id = model.Id, attributes = new Dictionary() { { "compoundAttr", newModel.CompoundAttr } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingDataTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingDataTests.cs index 5b58a05fe0..b1145f3654 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingDataTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingDataTests.cs @@ -55,8 +55,9 @@ public async Task Response400IfUpdatingNotSettableAttribute() var content = new { - datea = new + date = new { + id = todoItem.Id, type = "todo-items", attributes = new { @@ -90,6 +91,7 @@ public async Task Respond_404_If_EntityDoesNotExist() { data = new { + id = maxPersonId + 100, type = "todo-items", attributes = new { @@ -108,6 +110,41 @@ public async Task Respond_404_If_EntityDoesNotExist() Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } + [Fact] + public async Task Respond_400_If_IdNotInAttributeList() + { + // Arrange + var maxPersonId = _context.TodoItems.LastOrDefault()?.Id ?? 0; + var todoItem = _todoItemFaker.Generate(); + var builder = new WebHostBuilder() + .UseStartup(); + + var server = new TestServer(builder); + var client = server.CreateClient(); + + var content = new + { + data = new + { + type = "todo-items", + attributes = new + { + description = todoItem.Description, + ordinal = todoItem.Ordinal, + createdDate = DateTime.Now + } + } + }; + var request = PrepareRequest("PATCH", $"/api/v1/todo-items/{maxPersonId}", content); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(422, Convert.ToInt32(response.StatusCode)); + + } + [Fact] public async Task Can_Patch_Entity() @@ -129,6 +166,7 @@ public async Task Can_Patch_Entity() { data = new { + id = todoItem.Id, type = "todo-items", attributes = new { @@ -187,6 +225,8 @@ public async Task Patch_Entity_With_HasMany_Does_Not_Included_Relationships() data = new { type = "people", + id = person.Id, + attributes = new Dictionary { { "last-name", newPerson.LastName }, @@ -234,6 +274,7 @@ public async Task Can_Patch_Entity_And_HasOne_Relationships() data = new { type = "todo-items", + id = todoItem.Id, attributes = new { description = todoItem.Description, diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 6ba9c8ba6c..b1197447fd 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -549,6 +549,7 @@ public async Task Can_Delete_Relationship_By_Patching_Resource() { data = new { + id = todoItem.Id, type = "todo-items", relationships = new { diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs index cb9b52f9b2..7ce1604683 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/TodoItemsControllerTests.cs @@ -588,6 +588,7 @@ public async Task Can_Patch_TodoItem() { data = new { + id = todoItem.Id, type = "todo-items", attributes = new Dictionary() { @@ -639,6 +640,7 @@ public async Task Can_Patch_TodoItemWithNullable() { data = new { + id = todoItem.Id, type = "todo-items", attributes = new Dictionary() { @@ -690,6 +692,7 @@ public async Task Can_Patch_TodoItemWithNullValue() { data = new { + id = todoItem.Id, type = "todo-items", attributes = new Dictionary() { From 9139852b7afd2f997890f849bb705370d975ec58 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 11 Jun 2019 15:04:12 +0200 Subject: [PATCH 23/29] fix: separation tests --- .../Acceptance/UpdateTests.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/ResourceEntitySeparationExampleTests/Acceptance/UpdateTests.cs b/test/ResourceEntitySeparationExampleTests/Acceptance/UpdateTests.cs index c3a9cfdb78..eeb838c475 100644 --- a/test/ResourceEntitySeparationExampleTests/Acceptance/UpdateTests.cs +++ b/test/ResourceEntitySeparationExampleTests/Acceptance/UpdateTests.cs @@ -31,6 +31,7 @@ public async Task Can_Update_Course() { data = new { + id = course.Id, type = "courses", attributes = new Dictionary() { @@ -64,6 +65,7 @@ public async Task Can_Update_Department() { data = new { + id = dept.Id, type = "departments", attributes = new Dictionary() { @@ -97,6 +99,7 @@ public async Task Can_Update_Student() { data = new { + id = student.Id, type = "students", attributes = new Dictionary() { From 457e93dbb38784220565d687c799acadf82350d1 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 12 Jun 2019 10:52:37 +0200 Subject: [PATCH 24/29] chore: comments --- .../Data/DefaultEntityRepository.cs | 74 +++++++++++++------ .../Extensions/TypeExtensions.cs | 26 +++++++ .../Internal/InverseRelationships.cs | 6 ++ .../Models/RelationshipAttribute.cs | 1 - .../Serialization/JsonApiDeSerializer.cs | 16 ++-- 5 files changed, 94 insertions(+), 29 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index da98748dd0..29d9b0cfcb 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -62,7 +62,8 @@ public DefaultEntityRepository( _resourceDefinition = resourceDefinition; } - public DefaultEntityRepository( + public + DefaultEntityRepository( ILoggerFactory loggerFactory, IJsonApiContext jsonApiContext, IDbContextResolver contextResolver, @@ -171,6 +172,13 @@ public virtual async Task CreateAsync(TEntity entity) /// /// Loads the inverse relationships to prevent foreign key constraints from being violated /// to support implicit removes, see https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/502. + /// + /// example: + /// person.todoItems = [t1,t2] is updated to [t3, t4]. If t3, and/or t4 was + /// already related to a other person, and these persons are NOT loaded in to the + /// db context, then the query may cause a foreign key constraint. Loading + /// these "inverse relationships" into the DB context ensures EF core to take + /// this into account. /// private void LoadInverseRelationships(object trackedRelationshipValue, RelationshipAttribute relationshipAttr) { @@ -181,14 +189,15 @@ private void LoadInverseRelationships(object trackedRelationshipValue, Relations if (IsHasOneRelationship(hasOneAttr.InverseNavigation, trackedRelationshipValue.GetType())) { relationEntry.Reference(hasOneAttr.InverseNavigation).Load(); - } else + } + else { relationEntry.Collection(hasOneAttr.InverseNavigation).Load(); } } else if (relationshipAttr is HasManyAttribute hasManyAttr && !(relationshipAttr is HasManyThroughAttribute)) { - foreach (IIdentifiable relationshipValue in (IList)trackedRelationshipValue) + foreach (IIdentifiable relationshipValue in (IList)trackedRelationshipValue) { _context.Entry(relationshipValue).Reference(hasManyAttr.InverseNavigation).Load(); } @@ -198,9 +207,18 @@ private void LoadInverseRelationships(object trackedRelationshipValue, Relations private bool IsHasOneRelationship(string internalRelationshipName, Type type) { - var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.Single(r => r.InternalRelationshipName == internalRelationshipName); - if (relationshipAttr is HasOneAttribute) return true; - return false; + var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.SingleOrDefault(r => r.InternalRelationshipName == internalRelationshipName); + if (relationshipAttr != null) + { + if (relationshipAttr is HasOneAttribute) return true; + return false; + } + else + { + // relationshipAttr is null when there is not put a [RelationshipAttribute] on the inverse navigation property. + // In this case we use relfection to figure out what kind of relationship is pointing back. + return !(type.GetProperty(internalRelationshipName).PropertyType.Inherits(typeof(IEnumerable))); + } } @@ -247,23 +265,30 @@ public virtual async Task UpdateAsync(TId id, TEntity updatedEntity) /// public virtual async Task UpdateAsync(TEntity updatedEntity) { - var oldEntity = await GetAsync(updatedEntity.Id); - if (oldEntity == null) + var databaseEntity = await GetAsync(updatedEntity.Id); + if (databaseEntity == null) return null; foreach (var attr in _jsonApiContext.AttributesToUpdate.Keys) - attr.SetValue(oldEntity, attr.GetValue(updatedEntity)); + attr.SetValue(databaseEntity, attr.GetValue(updatedEntity)); foreach (var relationshipAttr in _jsonApiContext.RelationshipsToUpdate?.Keys) { - LoadCurrentRelationships(oldEntity, relationshipAttr); - var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out bool wasAlreadyTracked); + /// loads databasePerson.todoItems + LoadCurrentRelationships(databaseEntity, relationshipAttr); + /// trackedRelationshipValue is either equal to updatedPerson.todoItems + /// or replaced with the same set of todoItems from the EF Core change tracker, + /// if they were already tracked + object trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out bool wasAlreadyTracked); + /// loads into the db context any persons currently related + /// to the todoItems in trackedRelationshipValue LoadInverseRelationships(trackedRelationshipValue, relationshipAttr); - AssignRelationshipValue(oldEntity, trackedRelationshipValue, relationshipAttr); + /// assigns the updated relationship to the database entity + AssignRelationshipValue(databaseEntity, trackedRelationshipValue, relationshipAttr); } await _context.SaveChangesAsync(); - return oldEntity; + return databaseEntity; } @@ -271,7 +296,8 @@ public virtual async Task UpdateAsync(TEntity updatedEntity) /// Responsible for getting the relationship value for a given relationship /// attribute of a given entity. It ensures that the relationship value /// that it returns is attached to the database without reattaching duplicates instances - /// to the change tracker. + /// to the change tracker. It does so by checking if there already are + /// instances of the to-be-attached entities in the change tracker. /// private object GetTrackedRelationshipValue(RelationshipAttribute relationshipAttr, TEntity entity, out bool wasAlreadyAttached) { @@ -436,9 +462,16 @@ public async Task> ToListAsync(IQueryable entiti /// /// Before assigning new relationship values (UpdateAsync), we need to - /// attach the current relationship state to the dbcontext, else + /// attach the current database values of the relationship to the dbcontext, else /// it will not perform a complete-replace which is required for /// one-to-many and many-to-many. + /// + /// For example: a person `p1` has 2 todoitems: `t1` and `t2`. + /// If we want to update this todoitem set to `t3` and `t4`, simply assigning + /// `p1.todoItems = [t3, t4]` will result in EF Core adding them to the set, + /// resulting in `[t1 ... t4]`. Instead, we should first include `[t1, t2]`, + /// after which the reassignment `p1.todoItems = [t3, t4]` will actually + /// make EF Core perform a complete replace. This method does the loading of `[t1, t2]`. /// protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute relationshipAttribute) { @@ -454,21 +487,18 @@ protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute } /// - /// assigns relationships that were set in the request to the target entity of the request - /// todo: partially remove dependency on IJsonApiContext here: it is fine to - /// retrieve from the context WHICH relationships to update, but the actual - /// values should not come from the context. + /// Assigns the to /// - private void AssignRelationshipValue(TEntity oldEntity, object relationshipValue, RelationshipAttribute relationshipAttribute) + private void AssignRelationshipValue(TEntity targetEntity, object relationshipValue, RelationshipAttribute relationshipAttribute) { if (relationshipAttribute is HasManyThroughAttribute throughAttribute) { // todo: this logic should be put in the HasManyThrough attribute - AssignHasManyThrough(oldEntity, throughAttribute, (IList)relationshipValue); + AssignHasManyThrough(targetEntity, throughAttribute, (IList)relationshipValue); } else { - relationshipAttribute.SetValue(oldEntity, relationshipValue); + relationshipAttribute.SetValue(targetEntity, relationshipValue); } } diff --git a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs index d02f122061..b95694a495 100644 --- a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs @@ -8,6 +8,32 @@ namespace JsonApiDotNetCore.Extensions { internal static class TypeExtensions { + public static void AddRange(this IList list, IEnumerable items) + { + if (list == null) throw new ArgumentNullException(nameof(list)); + if (items == null) throw new ArgumentNullException(nameof(items)); + + if (list is List) + { + ((List)list).AddRange(items); + } + else + { + foreach (var item in items) + { + list.Add(item); + } + } + } + + + /// + /// Extension to use the LINQ cast method in a non-generic way: + /// + /// Type targetType = typeof(TEntity) + /// ((IList)myList).Cast(targetType). + /// + /// public static IEnumerable Cast(this IEnumerable source, Type type) { if (source == null) throw new ArgumentNullException(nameof(source)); diff --git a/src/JsonApiDotNetCore/Internal/InverseRelationships.cs b/src/JsonApiDotNetCore/Internal/InverseRelationships.cs index 02ad493910..bec80243d9 100644 --- a/src/JsonApiDotNetCore/Internal/InverseRelationships.cs +++ b/src/JsonApiDotNetCore/Internal/InverseRelationships.cs @@ -18,9 +18,14 @@ namespace JsonApiDotNetCore.Internal /// public interface IInverseRelationships { + /// + /// This method is called upon startup by JsonApiDotNetCore. It should + /// deal with resolving the inverse relationships. + /// void Resolve(); } + /// public class InverseRelationships : IInverseRelationships { private readonly ResourceGraph _graph; @@ -32,6 +37,7 @@ public InverseRelationships(IResourceGraph graph, IDbContextResolver resolver = _resolver = resolver; } + /// public void Resolve() { if (EntityFrameworkCoreIsEnabled()) diff --git a/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs b/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs index 5df2140cd7..1c484889c3 100644 --- a/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs +++ b/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs @@ -16,7 +16,6 @@ protected RelationshipAttribute(string publicName, Link documentLinks, bool canI public string PublicRelationshipName { get; internal set; } public string InternalRelationshipName { get; internal set; } - public string InverseNavigation { get; internal set; } /// diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index 27090c5bb7..5bd13a4aef 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -159,6 +159,7 @@ private object SetEntityAttributes( /// through the decoupling IJsonApiContext), we now no longer need to /// store the updated relationship values in this property. For now /// just assigning null as value, will remove this property later as a whole. + /// see #512 _jsonApiContext.AttributesToUpdate[attr] = null; } } @@ -224,14 +225,14 @@ private object SetHasOneRelationship(object entity, SetHasOneNavigationPropertyValue(entity, attr, rio, included); // recursive call ... - if(included != null) + if (included != null) { var navigationPropertyValue = attr.GetValue(entity); var resourceGraphEntity = _jsonApiContext.ResourceGraph.GetContextEntity(attr.Type); - if(navigationPropertyValue != null && resourceGraphEntity != null) + if (navigationPropertyValue != null && resourceGraphEntity != null) { var includedResource = included.SingleOrDefault(r => r.Type == rio.Type && r.Id == rio.Id); - if(includedResource != null) + if (includedResource != null) SetRelationships(navigationPropertyValue, resourceGraphEntity, includedResource.Relationships, included); } } @@ -257,7 +258,8 @@ private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, /// through the decoupling IJsonApiContext), we now no longer need to /// store the updated relationship values in this property. For now /// just assigning null as value, will remove this property later as a whole. - if (convertedValue == null ) _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); + /// see #512 + if (convertedValue == null) _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); } } @@ -281,6 +283,7 @@ private void SetHasOneNavigationPropertyValue(object entity, HasOneAttribute has /// through the decoupling IJsonApiContext), we now no longer need to /// store the updated relationship values in this property. For now /// just assigning null as value, will remove this property later as a whole. + /// see #512 _jsonApiContext.HasOneRelationshipPointers.Add(hasOneAttr, null); } } @@ -296,7 +299,7 @@ private object SetHasManyRelationship(object entity, if (relationships.TryGetValue(relationshipName, out RelationshipData relationshipData)) { - if(relationshipData.IsHasMany == false || relationshipData.ManyData == null) + if (relationshipData.IsHasMany == false || relationshipData.ManyData == null) return entity; var relatedResources = relationshipData.ManyData.Select(r => @@ -312,7 +315,8 @@ private object SetHasManyRelationship(object entity, /// through the decoupling IJsonApiContext), we now no longer need to /// store the updated relationship values in this property. For now /// just assigning null as value, will remove this property later as a whole. - _jsonApiContext.HasManyRelationshipPointers.Add(attr, null); + /// see #512 + _jsonApiContext.HasManyRelationshipPointers.Add(attr, null); } return entity; From 65f8a3a3e3819466fe1a078dc6b2f129d39ab56a Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 12 Jun 2019 11:54:12 +0200 Subject: [PATCH 25/29] Update DefaultEntityRepository.cs --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 29d9b0cfcb..a404225077 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -62,8 +62,7 @@ public DefaultEntityRepository( _resourceDefinition = resourceDefinition; } - public - DefaultEntityRepository( + public DefaultEntityRepository( ILoggerFactory loggerFactory, IJsonApiContext jsonApiContext, IDbContextResolver contextResolver, From 3ddb6a2490bdecf95764de45c1c4e292440c215e Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 17 Jun 2019 10:48:43 +0200 Subject: [PATCH 26/29] Update DefaultEntityRepository.cs --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index a404225077..427366b53e 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -171,8 +171,8 @@ public virtual async Task CreateAsync(TEntity entity) /// /// Loads the inverse relationships to prevent foreign key constraints from being violated /// to support implicit removes, see https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/502. - /// - /// example: + /// + /// Consider the following example: /// person.todoItems = [t1,t2] is updated to [t3, t4]. If t3, and/or t4 was /// already related to a other person, and these persons are NOT loaded in to the /// db context, then the query may cause a foreign key constraint. Loading From 2437077a4efbff7ffb871e599735e2c3265bb926 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 17 Jun 2019 10:55:18 +0200 Subject: [PATCH 27/29] Update TypeExtensions.cs --- src/JsonApiDotNetCore/Extensions/TypeExtensions.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs index b95694a495..9ebc942b03 100644 --- a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs @@ -8,6 +8,9 @@ namespace JsonApiDotNetCore.Extensions { internal static class TypeExtensions { + /// + /// Extension to use the LINQ AddRange method on an IList + /// public static void AddRange(this IList list, IEnumerable items) { if (list == null) throw new ArgumentNullException(nameof(list)); From 415306e2d839f9e952951175c914d8f2a994a281 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 17 Jun 2019 11:22:02 +0200 Subject: [PATCH 28/29] Update DefaultEntityRepository.cs --- 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 427366b53e..c5fa47b6ab 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -171,13 +171,14 @@ public virtual async Task CreateAsync(TEntity entity) /// /// Loads the inverse relationships to prevent foreign key constraints from being violated /// to support implicit removes, see https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/502. - /// + /// /// Consider the following example: /// person.todoItems = [t1,t2] is updated to [t3, t4]. If t3, and/or t4 was /// already related to a other person, and these persons are NOT loaded in to the /// db context, then the query may cause a foreign key constraint. Loading /// these "inverse relationships" into the DB context ensures EF core to take /// this into account. + /// /// private void LoadInverseRelationships(object trackedRelationshipValue, RelationshipAttribute relationshipAttr) { From 52a452d674cf4279274e823bca704b512bdf3725 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 17 Jun 2019 11:24:52 +0200 Subject: [PATCH 29/29] Update JsonApiReader.cs --- src/JsonApiDotNetCore/Formatters/JsonApiReader.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs b/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs index 15ce633cdf..b305bf9722 100644 --- a/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs +++ b/src/JsonApiDotNetCore/Formatters/JsonApiReader.cs @@ -83,6 +83,7 @@ public Task ReadAsync(InputFormatterContext context) } } + /// Checks if the deserialized payload has an ID included Checks if the elements in the deserialized payload have an ID included