fix: merge

Author: maurei

src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs

@@ -171,6 +171,14 @@ public virtual async Task<TEntity> CreateAsync(TEntity entity)
         /// <summary>
         /// 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.
+        /// <remark>
+        /// 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.
+        /// </remark>
         /// </summary>
         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();
                 }
@@ -199,13 +208,24 @@ private void LoadInverseRelationships(object trackedRelationshipValue, Relations
         private bool IsHasOneRelationship(string internalRelationshipName, Type type)
         {
             var relationshipAttr = _jsonApiContext.ResourceGraph.GetContextEntity(type).Relationships.SingleOrDefault(r => r.InternalRelationshipName == internalRelationshipName);
+<<<<<<< HEAD
             if(relationshipAttr != null)
             {
                 if (relationshipAttr is HasOneAttribute) return true;
                 return false;
             } else
             {
                 // relationshipAttr is null when we don't put a [RelationshipAttribute] on the inverse navigation property.
+=======
+            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.
+>>>>>>> master
                 // In this case we use relfection to figure out what kind of relationship is pointing back.
                 return !(type.GetProperty(internalRelationshipName).PropertyType.Inherits(typeof(IEnumerable)));
             }
@@ -255,31 +275,39 @@ public virtual async Task<TEntity> UpdateAsync(TId id, TEntity updatedEntity)
         /// <inheritdoc />
         public virtual async Task<TEntity> 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;
         }
 
 
         /// <summary>
         /// 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.
         /// </summary>
         private object GetTrackedRelationshipValue(RelationshipAttribute relationshipAttr, TEntity entity, out bool wasAlreadyAttached)
         {
@@ -445,9 +473,16 @@ public async Task<IReadOnlyList<TEntity>> ToListAsync(IQueryable<TEntity> entiti
 
         /// <summary>
         /// 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.
+        /// <para />
+        /// 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]`.
         /// </summary>
         protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute relationshipAttribute)
         {
@@ -463,21 +498,18 @@ protected void LoadCurrentRelationships(TEntity oldEntity, RelationshipAttribute
         }
 
         /// <summary>
-        /// 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 <paramref name="relationshipValue"/> to <paramref name="targetEntity"/>
         /// </summary>
-        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);
             }
         }
 

src/JsonApiDotNetCore/Extensions/TypeExtensions.cs

@@ -8,6 +8,10 @@ namespace JsonApiDotNetCore.Extensions
 {
     internal static class TypeExtensions
     {
+
+        /// <summary>
+        /// Extension to use the LINQ AddRange method on an IList
+        /// </summary>
         public static void AddRange<T>(this IList list, IEnumerable<T> items)
         {
             if (list == null) throw new ArgumentNullException(nameof(list));
@@ -25,7 +29,14 @@ public static void AddRange<T>(this IList list, IEnumerable<T> items)
                 }
             }
         }
-
+            
+        /// <summary>
+        /// Extension to use the LINQ cast method in a non-generic way:
+        /// <code>
+        /// Type targetType = typeof(TEntity)
+        /// ((IList)myList).Cast(targetType).
+        /// </code>
+        /// </summary>
         public static IEnumerable Cast(this IEnumerable source, Type type)
         {
             if (source == null) throw new ArgumentNullException(nameof(source));

src/JsonApiDotNetCore/Formatters/JsonApiReader.cs

@@ -83,6 +83,7 @@ public Task<InputFormatterResult> ReadAsync(InputFormatterContext context)
             }
         }
 
+        /// <summary> Checks if the deserialized payload has an ID included </summary
         private bool CheckForId(object model)
         {
             if (model == null) return false;
@@ -97,6 +98,7 @@ private bool CheckForId(object model)
             return false;
         }
 
+        /// <summary> Checks if the elements in the deserialized payload have an ID included </summary
         private bool CheckForId(IList modelList)
         {
             foreach (var model in modelList)

src/JsonApiDotNetCore/Internal/InverseRelationships.cs

@@ -18,9 +18,14 @@ namespace JsonApiDotNetCore.Internal
     /// </summary>
     public interface IInverseRelationships
     {
+        /// <summary>
+        /// This method is called upon startup by JsonApiDotNetCore. It should 
+        /// deal with resolving the inverse relationships. 
+        /// </summary>
         void Resolve();
     }
 
+    /// <inheritdoc />
     public class InverseRelationships : IInverseRelationships
     {
         private readonly ResourceGraph _graph;
@@ -32,6 +37,7 @@ public InverseRelationships(IResourceGraph graph, IDbContextResolver resolver =
             _resolver = resolver;
         }
 
+        /// <inheritdoc />
         public void Resolve()
         {
             if (EntityFrameworkCoreIsEnabled())

src/JsonApiDotNetCore/Internal/TypeHelper.cs

@@ -40,6 +40,9 @@ public static object ConvertType(object value, Type type)
                 if (type == typeof(DateTimeOffset))
                     return DateTimeOffset.Parse(stringValue);
 
+                if(type == typeof(TimeSpan)) 
+                    return TimeSpan.Parse(stringValue);
+
                 if (type.GetTypeInfo().IsEnum)
                     return Enum.Parse(type, stringValue);
 

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,16 @@ 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.DependentType);
                 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 +260,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 +285,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 +301,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 +317,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;

test/UnitTests/Internal/TypeHelper_Tests.cs

@@ -106,6 +106,30 @@ public void ConvertType_Returns_Default_Value_For_Empty_Strings()
                 Assert.Equal(t.Value, result);
             }
         }
+        
+        [Fact]
+        public void Can_Convert_TimeSpans() 
+        {
+            //arrange
+            TimeSpan timeSpan = TimeSpan.FromMinutes(45);
+            string stringSpan = timeSpan.ToString();
+
+            //act
+            var result = TypeHelper.ConvertType(stringSpan, typeof(TimeSpan));
+
+            //assert
+            Assert.Equal(timeSpan, result);
+        }
+
+        [Fact]
+        public void Bad_TimeSpanString_Throws() 
+        {
+            // arrange
+            var formattedString = "this_is_not_a_valid_timespan";
+
+            // act/assert
+            Assert.Throws<FormatException>(() => TypeHelper.ConvertType(formattedString, typeof(TimeSpan)));
+        }
 
         private enum TestEnum
         {