Cleanup collections

Author: Bart Koelman

src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs

@@ -139,7 +139,7 @@ protected virtual List<RelationshipAttribute> GetRelationships(Type entityType)
                     if (throughProperty == null)
                         throw new JsonApiSetupException($"Invalid {nameof(HasManyThroughAttribute)} on '{entityType}.{attribute.PropertyInfo.Name}': Resource does not contain a property named '{hasManyThroughAttribute.ThroughPropertyName}'.");
 
-                    var throughType = TryGetManyThroughType(throughProperty);
+                    var throughType = TryGetThroughType(throughProperty);
                     if (throughType == null)
                         throw new JsonApiSetupException($"Invalid {nameof(HasManyThroughAttribute)} on '{entityType}.{attribute.PropertyInfo.Name}': Referenced property '{throughProperty.Name}' does not implement 'ICollection<T>'.");
 
@@ -149,32 +149,32 @@ protected virtual List<RelationshipAttribute> GetRelationships(Type entityType)
                     // ArticleTag
                     hasManyThroughAttribute.ThroughType = throughType;
 
-                    var throughProperties = hasManyThroughAttribute.ThroughType.GetProperties();
+                    var throughProperties = throughType.GetProperties();
 
                     // ArticleTag.Article
                     hasManyThroughAttribute.LeftProperty = throughProperties.SingleOrDefault(x => x.PropertyType == entityType)
-                        ?? throw new JsonApiSetupException($"{hasManyThroughAttribute.ThroughType} does not contain a navigation property to type {entityType}");
+                        ?? throw new JsonApiSetupException($"{throughType} does not contain a navigation property to type {entityType}");
 
                     // ArticleTag.ArticleId
                     var leftIdPropertyName = JsonApiOptions.RelatedIdMapper.GetRelatedIdPropertyName(hasManyThroughAttribute.LeftProperty.Name);
                     hasManyThroughAttribute.LeftIdProperty = throughProperties.SingleOrDefault(x => x.Name == leftIdPropertyName)
-                        ?? throw new JsonApiSetupException($"{hasManyThroughAttribute.ThroughType} does not contain a relationship id property to type {entityType} with name {leftIdPropertyName}");
+                        ?? throw new JsonApiSetupException($"{throughType} does not contain a relationship id property to type {entityType} with name {leftIdPropertyName}");
 
                     // ArticleTag.Tag
                     hasManyThroughAttribute.RightProperty = throughProperties.SingleOrDefault(x => x.PropertyType == hasManyThroughAttribute.RightType)
-                        ?? throw new JsonApiSetupException($"{hasManyThroughAttribute.ThroughType} does not contain a navigation property to type {hasManyThroughAttribute.RightType}");
+                        ?? throw new JsonApiSetupException($"{throughType} does not contain a navigation property to type {hasManyThroughAttribute.RightType}");
 
                     // ArticleTag.TagId
                     var rightIdPropertyName = JsonApiOptions.RelatedIdMapper.GetRelatedIdPropertyName(hasManyThroughAttribute.RightProperty.Name);
                     hasManyThroughAttribute.RightIdProperty = throughProperties.SingleOrDefault(x => x.Name == rightIdPropertyName)
-                        ?? throw new JsonApiSetupException($"{hasManyThroughAttribute.ThroughType} does not contain a relationship id property to type {hasManyThroughAttribute.RightType} with name {rightIdPropertyName}");
+                        ?? throw new JsonApiSetupException($"{throughType} does not contain a relationship id property to type {hasManyThroughAttribute.RightType} with name {rightIdPropertyName}");
                 }
             }
 
             return attributes;
         }
 
-        private static Type TryGetManyThroughType(PropertyInfo throughProperty)
+        private static Type TryGetThroughType(PropertyInfo throughProperty)
         {
             if (throughProperty.PropertyType.IsGenericType)
             {

src/JsonApiDotNetCore/Extensions/TypeExtensions.cs

@@ -3,7 +3,6 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
-using System.Reflection;
 using JsonApiDotNetCore.Models;
 
 namespace JsonApiDotNetCore.Extensions
@@ -17,17 +16,10 @@ internal static class TypeExtensions
         /// ((IList)myList).CopyToList(targetType).
         /// </code>
         /// </summary>
-        public static IEnumerable CopyToList(this IEnumerable source, Type type)
+        public static IList CopyToList(this IEnumerable source, Type type)
         {
-            if (source == null) throw new ArgumentNullException(nameof(source));
-            if (type == null) throw new ArgumentNullException(nameof(type));
-
-            var list = (IList)Activator.CreateInstance(typeof(List<>).MakeGenericType(type));
-            foreach (var item in source.Cast<object>())
-            {
-                list.Add(TypeHelper.ConvertType(item, type));
-            }
-            return list;
+            Type collectionType = typeof(List<>).MakeGenericType(type);
+            return (IList)CopyToTypedCollection(source, collectionType);
         }
 
         /// <summary>

src/JsonApiDotNetCore/Graph/TypeLocator.cs

@@ -13,9 +13,8 @@ namespace JsonApiDotNetCore.Graph
     internal static class TypeLocator
     {
         /// <summary>
-        /// Determine whether or not this is a json:api resource by checking if it implements <see cref="IIdentifiable"/>.
-        /// Returns the status and the resultant id type, either `(true, Type)` OR `(false, null)`
-        /// </summary>        
+        /// Determine whether or not this is a json:api resource by checking if it implements <see cref="IIdentifiable{T}"/>.
+        /// </summary>
         public static Type GetIdType(Type resourceType)
         {
             var identifiableInterface = resourceType.GetInterfaces().FirstOrDefault(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IIdentifiable<>));

src/JsonApiDotNetCore/Hooks/Execution/HookExecutorHelper.cs

@@ -76,12 +76,12 @@ public IResourceHookContainer<TResource> GetResourceHookContainer<TResource>(Res
 
         public IEnumerable LoadDbValues(LeftType entityTypeForRepository, IEnumerable entities, ResourceHook hook, params RelationshipAttribute[] relationshipsToNextLayer)
         {
-            var idType = TypeHelper.GetIdentifierType(entityTypeForRepository);
+            var idType = TypeHelper.GetIdType(entityTypeForRepository);
             var parameterizedGetWhere = GetType()
                     .GetMethod(nameof(GetWhereAndInclude), BindingFlags.NonPublic | BindingFlags.Instance)
                     .MakeGenericMethod(entityTypeForRepository, idType);
             var cast = ((IEnumerable<object>)entities).Cast<IIdentifiable>();
-            var ids = cast.Select(e => e.StringId).CopyToList(idType);
+            var ids = cast.Select(e => TypeHelper.ConvertType(e.StringId, idType)).CopyToList(idType);
             var values = (IEnumerable)parameterizedGetWhere.Invoke(this, new object[] { ids, relationshipsToNextLayer });
             if (values == null) return null;
             return (IEnumerable)Activator.CreateInstance(typeof(HashSet<>).MakeGenericType(entityTypeForRepository), values.CopyToList(entityTypeForRepository));

src/JsonApiDotNetCore/Internal/Generics/RepositoryRelationshipUpdateHelper.cs

@@ -55,7 +55,7 @@ private async Task UpdateOneToOneAsync(IIdentifiable parent, RelationshipAttribu
             TRelatedResource value = null;
             if (relationshipIds.Any())
             {   // newOwner.id
-                var target = Expression.Constant(TypeHelper.ConvertType(relationshipIds.First(), TypeHelper.GetIdentifierType(relationship.RightType)));
+                var target = Expression.Constant(TypeHelper.ConvertType(relationshipIds.First(), TypeHelper.GetIdType(relationship.RightType)));
                 // (Person p) => ...
                 ParameterExpression parameter = Expression.Parameter(typeof(TRelatedResource));
                 // (Person p) => p.Id
@@ -80,7 +80,7 @@ private async Task UpdateOneToManyAsync(IIdentifiable parent, RelationshipAttrib
             {
                 // [1, 2, 3]
                 var target = Expression.Constant(TypeHelper.ConvertListType(relationshipIds,
-                    TypeHelper.GetIdentifierType(relationship.RightType)));
+                    TypeHelper.GetIdType(relationship.RightType)));
                 // (Person p) => ...
                 ParameterExpression parameter = Expression.Parameter(typeof(TRelatedResource));
                 // (Person p) => p.Id

src/JsonApiDotNetCore/Internal/TypeHelper.cs

@@ -5,30 +5,26 @@
 using System.Reflection;
 using System.Linq.Expressions;
 using JsonApiDotNetCore.Extensions;
+using JsonApiDotNetCore.Graph;
 using JsonApiDotNetCore.Models;
 
 namespace JsonApiDotNetCore.Internal
 {
     internal static class TypeHelper
     {
-        private static bool IsNullable(Type type)
-        {
-            return !type.IsValueType || Nullable.GetUnderlyingType(type) != null;
-        }
-
         public static object ConvertType(object value, Type type)
         {
-            if (value == null && !IsNullable(type))
+            if (value == null && !CanBeNull(type))
                 throw new FormatException("Cannot convert null to a non-nullable type");
 
             if (value == null)
                 return null;
 
-            Type typeOfValue = value.GetType();
+            Type runtimeType = value.GetType();
 
             try
             {
-                if (typeOfValue == type || type.IsAssignableFrom(typeOfValue))
+                if (runtimeType == type || type.IsAssignableFrom(runtimeType))
                     return value;
 
                 type = Nullable.GetUnderlyingType(type) ?? type;
@@ -44,7 +40,6 @@ public static object ConvertType(object value, Type type)
                 if (type == typeof(DateTimeOffset))
                     return DateTimeOffset.Parse(stringValue);
 
-
                 if (type == typeof(TimeSpan))
                     return TimeSpan.Parse(stringValue);
 
@@ -53,12 +48,17 @@ public static object ConvertType(object value, Type type)
 
                 return Convert.ChangeType(stringValue, type);
             }
-            catch (Exception e)
+            catch (Exception exception)
             {
-                throw new FormatException($"{typeOfValue} cannot be converted to {type}", e);
+                throw new FormatException($"Failed to convert '{value}' of type '{runtimeType}' to type '{type}'.", exception);
             }
         }
 
+        private static bool CanBeNull(Type type)
+        {
+            return !type.IsValueType || Nullable.GetUnderlyingType(type) != null;
+        }
+
         internal static object GetDefaultValue(this Type type)
         {
             return type.IsValueType ? type.New() : null;
@@ -238,19 +238,11 @@ public static Type ToConcreteCollectionType(this Type collectionType)
         /// <summary>
         /// Gets the type (Guid or int) of the Id of a type that implements IIdentifiable
         /// </summary>
-        public static Type GetIdentifierType(Type entityType)
-        {
-            var property = entityType.GetProperty("Id");
-            if (property == null) throw new ArgumentException("Type does not have a property Id");
-            return entityType.GetProperty("Id").PropertyType;
-        }
-
-        /// <summary>
-        /// Gets the type (Guid or int) of the Id of a type that implements IIdentifiable
-        /// </summary>
-        public static Type GetIdentifierType<T>() where T : IIdentifiable
+        public static Type GetIdType(Type resourceType)
         {
-            return typeof(T).GetProperty("Id").PropertyType;
+            var property = resourceType.GetProperty(nameof(Identifiable.Id));
+            if (property == null) throw new ArgumentException("Type does not have 'Id' property.");
+            return property.PropertyType;
         }
     }
 }

src/JsonApiDotNetCore/Models/Annotation/HasManyThroughAttribute.cs

@@ -4,7 +4,6 @@
 using System.Linq;
 using System.Reflection;
 using JsonApiDotNetCore.Extensions;
-using JsonApiDotNetCore.Internal;
 using JsonApiDotNetCore.Models.Links;
 
 namespace JsonApiDotNetCore.Models
@@ -76,22 +75,13 @@ public HasManyThroughAttribute(string publicName, string throughPropertyName, Li
         /// </summary>
         public override object GetValue(object entity)
         {
-            var throughNavigationProperty = entity.GetType()
-                .GetProperties()
-                .Single(p => p.Name == ThroughProperty.Name);
+            IEnumerable joinEntities = (IEnumerable)ThroughProperty.GetValue(entity) ?? Array.Empty<object>();
 
-            var throughEntities = throughNavigationProperty.GetValue(entity);
+            IEnumerable<object> rightEntities = joinEntities
+                .Cast<object>()
+                .Select(rightEntity =>  RightProperty.GetValue(rightEntity));
 
-            if (throughEntities == null)
-                // return an empty list for the right-type of the property.
-                return TypeHelper.CreateListFor(RightType);
-
-            // the right entities are included on the navigation/through entities. Extract and return them.
-            var rightEntities = new List<IIdentifiable>();
-            foreach (var rightEntity in (IEnumerable)throughEntities)
-                rightEntities.Add((IIdentifiable)RightProperty.GetValue(rightEntity));
-
-            return rightEntities.CopyToList(RightType);
+            return rightEntities.CopyToTypedCollection(PropertyInfo.PropertyType);
         }
 
         /// <inheritdoc />
@@ -105,16 +95,16 @@ public override void SetValue(object entity, object newValue)
             }
             else
             {
-                List<object> instances = new List<object>();
+                List<object> joinEntities = new List<object>();
                 foreach (IIdentifiable resource in (IEnumerable)newValue)
                 {
-                    object throughInstance = ThroughType.New();
-                    LeftProperty.SetValue(throughInstance, entity);
-                    RightProperty.SetValue(throughInstance, resource);
-                    instances.Add(throughInstance);
+                    object joinEntity = ThroughType.New();
+                    LeftProperty.SetValue(joinEntity, entity);
+                    RightProperty.SetValue(joinEntity, resource);
+                    joinEntities.Add(joinEntity);
                 }
 
-                var typedCollection = instances.CopyToTypedCollection(ThroughProperty.PropertyType);
+                var typedCollection = joinEntities.CopyToTypedCollection(ThroughProperty.PropertyType);
                 ThroughProperty.SetValue(entity, typedCollection);
             }
         }

test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingDataTests.cs

@@ -47,7 +47,11 @@ public async Task PatchResource_ModelWithEntityFrameworkInheritance_IsPatched()
         {
             // Arrange
             var dbContext = PrepareTest<Startup>();
-            var serializer = GetSerializer<SuperUser>(e => new { e.SecurityLevel, e.Username, e.Password });
+
+            var builder = new WebHostBuilder().UseStartup<Startup>();
+            var server = new TestServer(builder);
+
+            var serializer = TestFixture<Startup>.GetSerializer<SuperUser>(server.Host.Services, e => new { e.SecurityLevel, e.Username, e.Password });
             var superUser = new SuperUser { SecurityLevel = 1337, Username = "Super", Password = "User", LastPasswordChange = DateTime.Now.AddMinutes(-15) };
             dbContext.Set<SuperUser>().Add(superUser);
             dbContext.SaveChanges();
@@ -87,7 +91,7 @@ public async Task Response422IfUpdatingNotSettableAttribute()
             _context.TodoItems.Add(todoItem);
             _context.SaveChanges();
 
-            var serializer = _fixture.GetSerializer<TodoItem>(ti => new { ti.CalculatedValue });
+            var serializer = TestFixture<Startup>.GetSerializer<TodoItem>(server.Host.Services, ti => new { ti.CalculatedValue });
             var content = serializer.Serialize(todoItem);
             var request = PrepareRequest("PATCH", $"/api/v1/todoItems/{todoItem.Id}", content);
 
@@ -129,7 +133,7 @@ public async Task Respond_404_If_EntityDoesNotExist()
             var server = new TestServer(builder);
             var client = server.CreateClient();
 
-            var serializer = _fixture.GetSerializer<TodoItem>(ti => new { ti.Description, ti.Ordinal, ti.CreatedDate });
+            var serializer = TestFixture<Startup>.GetSerializer<TodoItem>(server.Host.Services, ti => new { ti.Description, ti.Ordinal, ti.CreatedDate });
             var content = serializer.Serialize(todoItem);
             var request = PrepareRequest("PATCH", $"/api/v1/todoItems/{todoItem.Id}", content);
 
@@ -159,7 +163,7 @@ public async Task Respond_422_If_IdNotInAttributeList()
 
             var server = new TestServer(builder);
             var client = server.CreateClient();
-            var serializer = _fixture.GetSerializer<TodoItem>(ti => new {ti.Description, ti.Ordinal, ti.CreatedDate});
+            var serializer = TestFixture<Startup>.GetSerializer<TodoItem>(server.Host.Services, ti => new {ti.Description, ti.Ordinal, ti.CreatedDate});
             var content = serializer.Serialize(todoItem);
             var request = PrepareRequest("PATCH", $"/api/v1/todoItems/{maxPersonId}", content);
 
@@ -194,7 +198,7 @@ public async Task Respond_409_If_IdInUrlIsDifferentFromIdInRequestBody()
             var builder = new WebHostBuilder().UseStartup<Startup>();
             var server = new TestServer(builder);
             var client = server.CreateClient();
-            var serializer = _fixture.GetSerializer<TodoItem>(ti => new {ti.Description, ti.Ordinal, ti.CreatedDate});
+            var serializer = TestFixture<Startup>.GetSerializer<TodoItem>(server.Host.Services, ti => new {ti.Description, ti.Ordinal, ti.CreatedDate});
             var content = serializer.Serialize(todoItem);
             var request = PrepareRequest("PATCH", $"/api/v1/todoItems/{wrongTodoItemId}", content);
 
@@ -263,7 +267,7 @@ public async Task Can_Patch_Entity()
             var builder = new WebHostBuilder().UseStartup<Startup>();
             var server = new TestServer(builder);
             var client = server.CreateClient();
-            var serializer = _fixture.GetSerializer<TodoItem>(p => new { p.Description, p.Ordinal });
+            var serializer = TestFixture<Startup>.GetSerializer<TodoItem>(server.Host.Services, p => new { p.Description, p.Ordinal });
 
             var request = PrepareRequest("PATCH", $"/api/v1/todoItems/{todoItem.Id}", serializer.Serialize(newTodoItem));
 
@@ -306,7 +310,7 @@ public async Task Patch_Entity_With_HasMany_Does_Not_Include_Relationships()
             var builder = new WebHostBuilder().UseStartup<Startup>();
             var server = new TestServer(builder);
             var client = server.CreateClient();
-            var serializer = _fixture.GetSerializer<Person>(p => new { p.LastName, p.FirstName });
+            var serializer = TestFixture<Startup>.GetSerializer<Person>(server.Host.Services, p => new { p.LastName, p.FirstName });
 
             var request = PrepareRequest("PATCH", $"/api/v1/people/{person.Id}", serializer.Serialize(newPerson));
 

test/JsonApiDotNetCoreExampleTests/Acceptance/TestFixture.cs

@@ -35,6 +35,15 @@ public TestFixture()
         public HttpClient Client { get; set; }
         public AppDbContext Context { get; private set; }
 
+        public static IRequestSerializer GetSerializer<TResource>(IServiceProvider serviceProvider, Expression<Func<TResource, dynamic>> attributes = null, Expression<Func<TResource, dynamic>> relationships = null) where TResource : class, IIdentifiable
+        {
+            var serializer = (IRequestSerializer)serviceProvider.GetService(typeof(IRequestSerializer));
+            var graph = (IResourceGraph)serviceProvider.GetService(typeof(IResourceGraph));
+            serializer.AttributesToSerialize = attributes != null ? graph.GetAttributes(attributes) : null;
+            serializer.RelationshipsToSerialize = relationships != null ? graph.GetRelationships(relationships) : null;
+            return serializer;
+        }
+
         public IRequestSerializer GetSerializer<TResource>(Expression<Func<TResource, dynamic>> attributes = null, Expression<Func<TResource, dynamic>> relationships = null) where TResource : class, IIdentifiable
         {
             var serializer = GetService<IRequestSerializer>();