fix(#312): deserializer not handling included relationships

jaredcnance · jaredcnance · commit 9dd7719193d7 · 2018-06-27T10:23:12.000-07:00
diff --git a/src/JsonApiDotNetCore/Builders/DocumentBuilder.cs b/src/JsonApiDotNetCore/Builders/DocumentBuilder.cs
@@ -262,11 +262,14 @@ private ResourceIdentifierObject GetRelationship(object entity)
             var objType = entity.GetType();
             var contextEntity = _jsonApiContext.ContextGraph.GetContextEntity(objType);
 
-            return new ResourceIdentifierObject
-            {
-                Type = contextEntity.EntityName,
-                Id = ((IIdentifiable)entity).StringId
-            };
+            if(entity is IIdentifiable identifiableEntity)
+                return new ResourceIdentifierObject
+                {
+                    Type = contextEntity.EntityName,
+                    Id = identifiableEntity.StringId
+                };
+
+            return null;
         }
 
         private ResourceIdentifierObject GetIndependentRelationshipIdentifier(HasOneAttribute hasOne, IIdentifiable entity)
diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs
@@ -185,17 +185,23 @@ public virtual async Task<IEnumerable<TEntity>> PageAsync(IQueryable<TEntity> en
 
         public async Task<int> CountAsync(IQueryable<TEntity> entities)
         {
-            return await entities.CountAsync();
+            return (entities is IAsyncEnumerable<TEntity>)
+                 ? await entities.CountAsync()
+                 : entities.Count();
         }
 
-        public Task<TEntity> FirstOrDefaultAsync(IQueryable<TEntity> entities)
+        public async Task<TEntity> FirstOrDefaultAsync(IQueryable<TEntity> entities)
         {
-            return entities.FirstOrDefaultAsync();
+            return (entities is IAsyncEnumerable<TEntity>)
+               ? await entities.FirstOrDefaultAsync()
+               : entities.FirstOrDefault();
         }
 
         public async Task<IReadOnlyList<TEntity>> ToListAsync(IQueryable<TEntity> entities)
         {
-            return await entities.ToListAsync();
+            return (entities is IAsyncEnumerable<TEntity>)
+                ? await entities.ToListAsync()
+                : entities.ToList();
         }
     }
 }
diff --git a/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs b/src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs
@@ -136,7 +136,6 @@ public static void AddJsonApiInternals(
             services.AddScoped<IJsonApiReader, JsonApiReader>();
             services.AddScoped<IGenericProcessorFactory, GenericProcessorFactory>();
             services.AddScoped(typeof(GenericProcessor<>));
-            services.AddScoped(typeof(GenericProcessor<,>));
             services.AddScoped<IQueryAccessor, QueryAccessor>();
             services.AddScoped<IQueryParser, QueryParser>();
             services.AddScoped<IControllerContext, Services.ControllerContext>();
diff --git a/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs b/src/JsonApiDotNetCore/Extensions/TypeExtensions.cs
@@ -1,4 +1,5 @@
-﻿using System;
+using JsonApiDotNetCore.Internal;
+using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
@@ -51,8 +52,20 @@ public static TInterface New<TInterface>(this Type t)
         {
             if (t == null) throw new ArgumentNullException(nameof(t));
 
-            var instance = (TInterface)Activator.CreateInstance(t);
+            var instance = (TInterface)CreateNewInstance(t);
             return instance;
         }
+
+        private static object CreateNewInstance(Type type)
+        {
+            try
+            {
+                return Activator.CreateInstance(type);
+            }
+            catch (Exception e)
+            {
+                throw new JsonApiException(500, $"Type '{type}' cannot be instantiated using the default constructor.", e);
+            }
+        }
     }
 }
diff --git a/src/JsonApiDotNetCore/Internal/Generics/GenericProcessor.cs b/src/JsonApiDotNetCore/Internal/Generics/GenericProcessor.cs
@@ -14,12 +14,7 @@ public interface IGenericProcessor
         void SetRelationships(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds);
     }
 
-    public class GenericProcessor<T> : GenericProcessor<T, int> where T : class, IIdentifiable<int>
-    {
-        public GenericProcessor(IDbContextResolver contextResolver) : base(contextResolver) { }
-    }
-
-    public class GenericProcessor<T, TId> : IGenericProcessor where T : class, IIdentifiable<TId>
+    public class GenericProcessor<T> : IGenericProcessor where T : class, IIdentifiable
     {
         private readonly DbContext _context;
         public GenericProcessor(IDbContextResolver contextResolver)
@@ -38,12 +33,12 @@ public void SetRelationships(object parent, RelationshipAttribute relationship,
         {
             if (relationship.IsHasMany)
             {
-                var entities = _context.GetDbSet<T>().Where(x => relationshipIds.Contains(x.StringId)).ToList();
+                var entities = _context.Set<T>().Where(x => relationshipIds.Contains(x.StringId)).ToList();
                 relationship.SetValue(parent, entities);
             }
             else
             {
-                var entity = _context.GetDbSet<T>().SingleOrDefault(x => relationshipIds.First() == x.StringId);
+                var entity = _context.Set<T>().SingleOrDefault(x => relationshipIds.First() == x.StringId);
                 relationship.SetValue(parent, entity);
             }
         }
diff --git a/src/JsonApiDotNetCore/Models/Identifiable.cs b/src/JsonApiDotNetCore/Models/Identifiable.cs
@@ -15,7 +15,7 @@ public class Identifiable<T> : IIdentifiable<T>
         public string StringId
         {
             get => GetStringId(Id);
-            set => Id = (T)GetConcreteId(value);
+            set => Id = GetTypedId(value);
         }
 
         protected virtual string GetStringId(object value)
@@ -34,6 +34,13 @@ protected virtual string GetStringId(object value)
                 : stringValue;
         }
 
+        protected virtual T GetTypedId(string value)
+        {
+            var convertedValue = TypeHelper.ConvertType(value, typeof(T));
+            return convertedValue == null ? default : (T)convertedValue;
+        }
+
+        [Obsolete("Use GetTypedId instead")]
         protected virtual object GetConcreteId(string value)
         {
             return TypeHelper.ConvertType(value, typeof(T));
diff --git a/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs b/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs
@@ -13,6 +13,17 @@ protected RelationshipAttribute(string publicName, Link documentLinks, bool canI
 
         public string PublicRelationshipName { get; }
         public string InternalRelationshipName { get; internal set; }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        /// 
+        /// <example>
+        /// <code>
+        /// public List&lt;Articles&gt; Articles { get; set; } // Type => Article
+        /// </code>
+        /// </example>
         public Type Type { get; internal set; }
         public bool IsHasMany => GetType() == typeof(HasManyAttribute);
         public bool IsHasOne => GetType() == typeof(HasOneAttribute);
diff --git a/src/JsonApiDotNetCore/Serialization/IJsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/IJsonApiDeSerializer.cs
@@ -9,6 +9,6 @@ public interface IJsonApiDeSerializer
         TEntity Deserialize<TEntity>(string requestBody);
         object DeserializeRelationship(string requestBody);
         List<TEntity> DeserializeList<TEntity>(string requestBody);
-        object DocumentToObject(DocumentData data);
+        object DocumentToObject(DocumentData data, List<DocumentData> included = null);
     }
-}
+}
diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs
@@ -54,7 +54,7 @@ public object Deserialize(string requestBody)
                 var document = bodyJToken.ToObject<Document>();
 
                 _jsonApiContext.DocumentMeta = document.Meta;
-                var entity = DocumentToObject(document.Data);
+                var entity = DocumentToObject(document.Data, document.Included);
                 return entity;
             }
             catch (Exception e)
@@ -95,8 +95,8 @@ public List<TEntity> DeserializeList<TEntity>(string requestBody)
                 var deserializedList = new List<TEntity>();
                 foreach (var data in documents.Data)
                 {
-                    var entity = DocumentToObject(data);
-                    deserializedList.Add((TEntity)entity);
+                    var entity = (TEntity)DocumentToObject(data, documents.Included);
+                    deserializedList.Add(entity);
                 }
 
                 return deserializedList;
@@ -107,7 +107,7 @@ public List<TEntity> DeserializeList<TEntity>(string requestBody)
             }
         }
 
-        public object DocumentToObject(DocumentData data)
+        public object DocumentToObject(DocumentData data, List<DocumentData> included = null)
         {
             if (data == null) throw new JsonApiException(422, "Failed to deserialize document as json:api.");
 
@@ -117,7 +117,7 @@ public object DocumentToObject(DocumentData data)
             var entity = Activator.CreateInstance(contextEntity.EntityType);
 
             entity = SetEntityAttributes(entity, contextEntity, data.Attributes);
-            entity = SetRelationships(entity, contextEntity, data.Relationships);
+            entity = SetRelationships(entity, contextEntity, data.Relationships, included);
 
             var identifiableEntity = (IIdentifiable)entity;
 
@@ -172,7 +172,8 @@ private object DeserializeComplexType(JContainer obj, Type targetType)
         private object SetRelationships(
             object entity,
             ContextEntity contextEntity,
-            Dictionary<string, RelationshipData> relationships)
+            Dictionary<string, RelationshipData> relationships, 
+            List<DocumentData> included = null)
         {
             if (relationships == null || relationships.Count == 0)
                 return entity;
@@ -182,8 +183,8 @@ private object SetRelationships(
             foreach (var attr in contextEntity.Relationships)
             {
                 entity = attr.IsHasOne
-                    ? SetHasOneRelationship(entity, entityProperties, (HasOneAttribute)attr, contextEntity, relationships)
-                    : SetHasManyRelationship(entity, entityProperties, attr, contextEntity, relationships);
+                    ? SetHasOneRelationship(entity, entityProperties, (HasOneAttribute)attr, contextEntity, relationships, included)
+                    : SetHasManyRelationship(entity, entityProperties, attr, contextEntity, relationships, included);
             }
 
             return entity;
@@ -193,39 +194,53 @@ private object SetHasOneRelationship(object entity,
             PropertyInfo[] entityProperties,
             HasOneAttribute attr,
             ContextEntity contextEntity,
-            Dictionary<string, RelationshipData> relationships)
+            Dictionary<string, RelationshipData> relationships,
+            List<DocumentData> included = null)
         {
             var relationshipName = attr.PublicRelationshipName;
 
-            if (relationships.TryGetValue(relationshipName, out RelationshipData relationshipData))
-            {
-                var relationshipAttr = _jsonApiContext.RequestEntity.Relationships
-                    .SingleOrDefault(r => r.PublicRelationshipName == relationshipName);
+            if (relationships.TryGetValue(relationshipName, out RelationshipData relationshipData) == false)
+                return entity;
+            
+            var relationshipAttr = _jsonApiContext.RequestEntity.Relationships
+                .SingleOrDefault(r => r.PublicRelationshipName == relationshipName);
 
-                if (relationshipAttr == null)
-                    throw new JsonApiException(400, $"{_jsonApiContext.RequestEntity.EntityName} does not contain a relationship '{relationshipName}'");
+            if (relationshipAttr == null)
+                throw new JsonApiException(400, $"{_jsonApiContext.RequestEntity.EntityName} does not contain a relationship '{relationshipName}'");
 
-                var rio = (ResourceIdentifierObject)relationshipData.ExposedData;
+            var rio = (ResourceIdentifierObject)relationshipData.ExposedData;
 
-                var foreignKey = attr.IdentifiablePropertyName;
-                var entityProperty = entityProperties.FirstOrDefault(p => p.Name == foreignKey);
-                if (entityProperty == null && rio != null)
-                    throw new JsonApiException(400, $"{contextEntity.EntityType.Name} does not contain a foreign key property '{foreignKey}' for has one relationship '{attr.InternalRelationshipName}'");
+            var foreignKey = attr.IdentifiablePropertyName;
+            var foreignKeyProperty = entityProperties.FirstOrDefault(p => p.Name == foreignKey);
 
-                if (entityProperty != null)
-                {
-                    // e.g. PATCH /articles
-                    // {... { "relationships":{ "Owner": { "data" :null } } } }
-                    if (rio == null && Nullable.GetUnderlyingType(entityProperty.PropertyType) == null)
-                        throw new JsonApiException(400, $"Cannot set required relationship identifier '{attr.IdentifiablePropertyName}' to null.");
+            if (foreignKeyProperty == null && rio == null)
+                return entity;
 
-                    var newValue = rio?.Id ?? null;
-                    var convertedValue = TypeHelper.ConvertType(newValue, entityProperty.PropertyType);
+            if (foreignKeyProperty == null && rio != null)
+                throw new JsonApiException(400, $"{contextEntity.EntityType.Name} does not contain a foreign key property '{foreignKey}' for has one relationship '{attr.InternalRelationshipName}'");
 
-                    _jsonApiContext.RelationshipsToUpdate[relationshipAttr] = convertedValue;
+            // e.g. PATCH /articles
+            // {... { "relationships":{ "Owner": { "data": null } } } }
+            if (rio == null && Nullable.GetUnderlyingType(foreignKeyProperty.PropertyType) == null)
+                throw new JsonApiException(400, $"Cannot set required relationship identifier '{attr.IdentifiablePropertyName}' to null because it is a non-nullable type.");
 
-                    entityProperty.SetValue(entity, convertedValue);
-                }
+            var newValue = rio?.Id ?? null;
+            var convertedValue = TypeHelper.ConvertType(newValue, foreignKeyProperty.PropertyType);
+
+            _jsonApiContext.RelationshipsToUpdate[relationshipAttr] = convertedValue;
+
+            foreignKeyProperty.SetValue(entity, convertedValue);
+
+
+            if(rio != null
+                // if the resource identifier is null, there should be no reason to instantiate an instance
+                && rio.Id != null)
+            {
+                // we have now set the FK property on the resource, now we need to check to see if the
+                // related entity was included in the payload and update its attributes
+                var includedRelationshipObject = GetIncludedRelationship(rio, included, relationshipAttr);
+                if (includedRelationshipObject != null)
+                    relationshipAttr.SetValue(entity, includedRelationshipObject);
             }
 
             return entity;
@@ -235,7 +250,8 @@ private object SetHasManyRelationship(object entity,
             PropertyInfo[] entityProperties,
             RelationshipAttribute attr,
             ContextEntity contextEntity,
-            Dictionary<string, RelationshipData> relationships)
+            Dictionary<string, RelationshipData> relationships,
+            List<DocumentData> included = null)
         {
             var relationshipName = attr.PublicRelationshipName;
 
@@ -245,14 +261,13 @@ private object SetHasManyRelationship(object entity,
 
                 if (data == null) return entity;
 
-                var relationshipShells = relationshipData.ManyData.Select(r =>
+                var relatedResources = relationshipData.ManyData.Select(r =>
                 {
-                    var instance = attr.Type.New<IIdentifiable>();
-                    instance.StringId = r.Id;
+                    var instance = GetIncludedRelationship(r, included, attr);
                     return instance;
                 });
 
-                var convertedCollection = TypeHelper.ConvertCollection(relationshipShells, attr.Type);
+                var convertedCollection = TypeHelper.ConvertCollection(relatedResources, attr.Type);
 
                 attr.SetValue(entity, convertedCollection);
 
@@ -261,5 +276,41 @@ private object SetHasManyRelationship(object entity,
 
             return entity;
         }
+
+        private IIdentifiable GetIncludedRelationship(ResourceIdentifierObject relatedResourceIdentifier, List<DocumentData> includedResources, RelationshipAttribute relationshipAttr)
+        {
+            // at this point we can be sure the relationshipAttr.Type is IIdentifiable because we were able to successfully build the ContextGraph
+            var relatedInstance = relationshipAttr.Type.New<IIdentifiable>();
+            relatedInstance.StringId = relatedResourceIdentifier.Id;
+
+            // can't provide any more data other than the rio since it is not contained in the included section
+            if (includedResources == null || includedResources.Count == 0)
+                return relatedInstance;
+
+            var includedResource = GetLinkedResource(relatedResourceIdentifier, includedResources);
+            if (includedResource == null)
+                return relatedInstance;
+
+            var contextEntity = _jsonApiContext.ContextGraph.GetContextEntity(relationshipAttr.Type);
+            if (contextEntity == null)
+                throw new JsonApiException(400, $"Included type '{relationshipAttr.Type}' is not a registered json:api resource.");
+            
+            SetEntityAttributes(relatedInstance, contextEntity, includedResource.Attributes);
+
+            return relatedInstance;
+        }
+
+        private DocumentData GetLinkedResource(ResourceIdentifierObject relatedResourceIdentifier, List<DocumentData> includedResources)
+        {
+            try
+            {
+                return includedResources.SingleOrDefault(r => r.Type == relatedResourceIdentifier.Type && r.Id == relatedResourceIdentifier.Id);
+            }
+            catch (InvalidOperationException e)
+            {
+                throw new JsonApiException(400, $"A compound document MUST NOT include more than one resource object for each type and id pair."
+                        + $"The duplicate pair was '{relatedResourceIdentifier.Type}, {relatedResourceIdentifier.Id}'", e);
+            }
+        }
     }
 }
diff --git a/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs b/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs
diff --git a/test/UnitTests/Services/Operations/Processors/CreateOpProcessorTests.cs b/test/UnitTests/Services/Operations/Processors/CreateOpProcessorTests.cs

Original file line number	Diff line number	Diff line change
`@@ -185,17 +185,23 @@ public virtual async Task<IEnumerable<TEntity>> PageAsync(IQueryable<TEntity> en`
`185`	`185`
`186`	`186`	`public async Task<int> CountAsync(IQueryable<TEntity> entities)`
`187`	`187`	`{`
`188`		`- return await entities.CountAsync();`
	`188`	`+ return (entities is IAsyncEnumerable<TEntity>)`
	`189`	`+ ? await entities.CountAsync()`
	`190`	`+ : entities.Count();`
`189`	`191`	`}`
`190`	`192`
`191`		`- public Task<TEntity> FirstOrDefaultAsync(IQueryable<TEntity> entities)`
	`193`	`+ public async Task<TEntity> FirstOrDefaultAsync(IQueryable<TEntity> entities)`
`192`	`194`	`{`
`193`		`- return entities.FirstOrDefaultAsync();`
	`195`	`+ return (entities is IAsyncEnumerable<TEntity>)`
	`196`	`+ ? await entities.FirstOrDefaultAsync()`
	`197`	`+ : entities.FirstOrDefault();`
`194`	`198`	`}`
`195`	`199`
`196`	`200`	`public async Task<IReadOnlyList<TEntity>> ToListAsync(IQueryable<TEntity> entities)`
`197`	`201`	`{`
`198`		`- return await entities.ToListAsync();`
	`202`	`+ return (entities is IAsyncEnumerable<TEntity>)`
	`203`	`+ ? await entities.ToListAsync()`
	`204`	`+ : entities.ToList();`
`199`	`205`	`}`
`200`	`206`	`}`
`201`	`207`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,12 +14,7 @@ public interface IGenericProcessor`
`14`	`14`	`void SetRelationships(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds);`
`15`	`15`	`}`
`16`	`16`
`17`		`- public class GenericProcessor<T> : GenericProcessor<T, int> where T : class, IIdentifiable<int>`
`18`		`- {`
`19`		`- public GenericProcessor(IDbContextResolver contextResolver) : base(contextResolver) { }`
`20`		`- }`
`21`		`-`
`22`		`- public class GenericProcessor<T, TId> : IGenericProcessor where T : class, IIdentifiable<TId>`
	`17`	`+ public class GenericProcessor<T> : IGenericProcessor where T : class, IIdentifiable`
`23`	`18`	`{`
`24`	`19`	`private readonly DbContext _context;`
`25`	`20`	`public GenericProcessor(IDbContextResolver contextResolver)`
`@@ -38,12 +33,12 @@ public void SetRelationships(object parent, RelationshipAttribute relationship,`
`38`	`33`	`{`
`39`	`34`	`if (relationship.IsHasMany)`
`40`	`35`	`{`
`41`		`- var entities = _context.GetDbSet<T>().Where(x => relationshipIds.Contains(x.StringId)).ToList();`
	`36`	`+ var entities = _context.Set<T>().Where(x => relationshipIds.Contains(x.StringId)).ToList();`
`42`	`37`	`relationship.SetValue(parent, entities);`
`43`	`38`	`}`
`44`	`39`	`else`
`45`	`40`	`{`
`46`		`- var entity = _context.GetDbSet<T>().SingleOrDefault(x => relationshipIds.First() == x.StringId);`
	`41`	`+ var entity = _context.Set<T>().SingleOrDefault(x => relationshipIds.First() == x.StringId);`
`47`	`42`	`relationship.SetValue(parent, entity);`
`48`	`43`	`}`
`49`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ public class Identifiable<T> : IIdentifiable<T>`
`15`	`15`	`public string StringId`
`16`	`16`	`{`
`17`	`17`	`get => GetStringId(Id);`
`18`		`- set => Id = (T)GetConcreteId(value);`
	`18`	`+ set => Id = GetTypedId(value);`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`protected virtual string GetStringId(object value)`
`@@ -34,6 +34,13 @@ protected virtual string GetStringId(object value)`
`34`	`34`	`: stringValue;`
`35`	`35`	`}`
`36`	`36`
	`37`	`+ protected virtual T GetTypedId(string value)`
	`38`	`+ {`
	`39`	`+ var convertedValue = TypeHelper.ConvertType(value, typeof(T));`
	`40`	`+ return convertedValue == null ? default : (T)convertedValue;`
	`41`	`+ }`
	`42`	`+`
	`43`	`+ [Obsolete("Use GetTypedId instead")]`
`37`	`44`	`protected virtual object GetConcreteId(string value)`
`38`	`45`	`{`
`39`	`46`	`return TypeHelper.ConvertType(value, typeof(T));`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,6 @@ public interface IJsonApiDeSerializer`
`9`	`9`	`TEntity Deserialize<TEntity>(string requestBody);`
`10`	`10`	`object DeserializeRelationship(string requestBody);`
`11`	`11`	`List<TEntity> DeserializeList<TEntity>(string requestBody);`
`12`		`- object DocumentToObject(DocumentData data);`
	`12`	`+ object DocumentToObject(DocumentData data, List<DocumentData> included = null);`
`13`	`13`	`}`
`14`		`-}`
	`14`	`+}`