Cleaning repository of remaining business logic (#579)

* feat: first draft of attribute diffing in repo layer

* chore: remove EF Core-specific DetachRelationshipPointers from repository interface

* chore: replace SingleOrDefaultAsync with FirstOrDefaultAsync for increased performance

* chore: decoupled ResourceDefinition from repository layer in Filter implementation

* chore: remove updatevaluehelper

* fix: Can_Patch_Entity test

* fix: EntityResourceTest, variable name updatedFields -> targetedFields

* chore: add comment referencing to github issue

* chore: rename pageManager -> pageService

* chore: remove deprecations

* fix: undid wrong replace all of BeforeUpdate -> pageService

* chore: remove unreferenced method in ResponseSerializer

Author: maurei

src/Examples/JsonApiDotNetCoreExample/Services/CustomArticleService.cs

@@ -3,10 +3,7 @@
 using JsonApiDotNetCore.Hooks;
 using JsonApiDotNetCore.Internal;
 using JsonApiDotNetCore.Internal.Contracts;
-using JsonApiDotNetCore.Managers.Contracts;
-using JsonApiDotNetCore.Models;
 using JsonApiDotNetCore.Query;
-using JsonApiDotNetCore.Serialization;
 using JsonApiDotNetCore.Services;
 using JsonApiDotNetCoreExample.Models;
 using Microsoft.Extensions.Logging;
@@ -20,15 +17,14 @@ public CustomArticleService(ISortService sortService,
                                     IFilterService filterService,
                                     IEntityRepository<Article, int> repository,
                                     IJsonApiOptions options,
-                                    ICurrentRequest currentRequest,
                                     IIncludeService includeService,
                                     ISparseFieldsService sparseFieldsService,
-                                    IPageService pageManager,
+                                    IPageService pageService,
                                     IResourceGraph resourceGraph,
                                     IResourceHookExecutor hookExecutor = null,
                                     ILoggerFactory loggerFactory = null)
-            : base(sortService, filterService, repository, options, currentRequest, includeService, sparseFieldsService,
-                   pageManager, resourceGraph, hookExecutor, loggerFactory)
+            : base(sortService, filterService, repository, options, includeService, sparseFieldsService,
+                   pageService, resourceGraph, hookExecutor, loggerFactory)
         {
         }
 

src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs

@@ -4,11 +4,9 @@
 using System.Linq;
 using System.Threading.Tasks;
 using JsonApiDotNetCore.Extensions;
-using JsonApiDotNetCore.Internal;
 using JsonApiDotNetCore.Internal.Contracts;
 using JsonApiDotNetCore.Internal.Generics;
 using JsonApiDotNetCore.Internal.Query;
-using JsonApiDotNetCore.Managers.Contracts;
 using JsonApiDotNetCore.Models;
 using JsonApiDotNetCore.Serialization;
 using Microsoft.EntityFrameworkCore;
@@ -20,52 +18,42 @@ namespace JsonApiDotNetCore.Data
     /// Provides a default repository implementation and is responsible for
     /// abstracting any EF Core APIs away from the service layer.
     /// </summary>
-    public class DefaultEntityRepository<TEntity, TId>
-        : IEntityRepository<TEntity, TId>,
-        IEntityFrameworkRepository<TEntity>
+    public class DefaultEntityRepository<TEntity, TId> : IEntityRepository<TEntity, TId>
         where TEntity : class, IIdentifiable<TId>
     {
-        private readonly ICurrentRequest _currentRequest;
         private readonly ITargetedFields _targetedFields;
         private readonly DbContext _context;
         private readonly DbSet<TEntity> _dbSet;
-        private readonly ILogger _logger;
         private readonly IResourceGraph _resourceGraph;
         private readonly IGenericProcessorFactory _genericProcessorFactory;
-        private readonly ResourceDefinition<TEntity> _resourceDefinition;
 
         public DefaultEntityRepository(
-            ICurrentRequest currentRequest,
-            ITargetedFields updatedFields,
+            ITargetedFields targetedFields,
             IDbContextResolver contextResolver,
             IResourceGraph resourceGraph,
-            IGenericProcessorFactory genericProcessorFactory,
-            ResourceDefinition<TEntity> resourceDefinition = null)
-            : this(currentRequest, updatedFields, contextResolver, resourceGraph, genericProcessorFactory, resourceDefinition, null)
+            IGenericProcessorFactory genericProcessorFactory)
+            : this(targetedFields, contextResolver, resourceGraph, genericProcessorFactory, null)
         { }
 
         public DefaultEntityRepository(
-            ICurrentRequest currentRequest,
-            ITargetedFields updatedFields,
+            ITargetedFields targetedFields,
             IDbContextResolver contextResolver,
             IResourceGraph resourceGraph,
             IGenericProcessorFactory genericProcessorFactory,
-            ResourceDefinition<TEntity> resourceDefinition = null,
             ILoggerFactory loggerFactory = null)
         {
-            _logger = loggerFactory?.CreateLogger<DefaultEntityRepository<TEntity, TId>>();
-            _currentRequest = currentRequest;
-            _targetedFields = updatedFields;
+            _targetedFields = targetedFields;
             _resourceGraph = resourceGraph;
             _genericProcessorFactory = genericProcessorFactory;
             _context = contextResolver.GetContext();
             _dbSet = _context.Set<TEntity>();
-            _resourceDefinition = resourceDefinition;
         }
 
         /// <inheritdoc />
         public virtual IQueryable<TEntity> Get() => _dbSet;
-        
+        /// <inheritdoc />
+        public virtual IQueryable<TEntity> Get(TId id) => _dbSet.Where(e => e.Id.Equals(id));
+
         /// <inheritdoc />
         public virtual IQueryable<TEntity> Select(IQueryable<TEntity> entities, List<AttrAttribute> fields)
         {
@@ -79,12 +67,9 @@ public virtual IQueryable<TEntity> Select(IQueryable<TEntity> entities, List<Att
         public virtual IQueryable<TEntity> Filter(IQueryable<TEntity> entities, FilterQueryContext filterQueryContext)
         {
             if (filterQueryContext.IsCustom)
-            {   // todo: consider to move this business logic to service layer
-                var filterQuery = filterQueryContext.Query;
-                var defaultQueryFilters = _resourceDefinition.GetQueryFilters();
-                if (defaultQueryFilters != null && defaultQueryFilters.TryGetValue(filterQuery.Target, out var defaultQueryFilter) == true)
-                    return defaultQueryFilter(entities, filterQuery);
-
+            {
+                var query = (Func<IQueryable<TEntity>, FilterQuery, IQueryable<TEntity>>)filterQueryContext.CustomQuery;
+                return query(entities, filterQueryContext.Query);
             }
             return entities.Filter(filterQueryContext);
         }
@@ -95,45 +80,30 @@ public virtual IQueryable<TEntity> Sort(IQueryable<TEntity> entities, SortQueryC
             return entities.Sort(sortQueryContext);
         }
 
-        /// <inheritdoc />
-        public virtual async Task<TEntity> GetAsync(TId id, List<AttrAttribute> fields = null)
-        {
-            return await Select(Get(), fields).SingleOrDefaultAsync(e => e.Id.Equals(id));
-        }
-
-        /// <inheritdoc />
-        public virtual async Task<TEntity> GetAndIncludeAsync(TId id, RelationshipAttribute relationship, List<AttrAttribute> fields = null)
-        {
-            _logger?.LogDebug($"[JADN] GetAndIncludeAsync({id}, {relationship.PublicRelationshipName})");
-            var includedSet = Include(Select(Get(), fields), relationship);
-            var result = await includedSet.SingleOrDefaultAsync(e => e.Id.Equals(id));
-            return result;
-        }
-
         /// <inheritdoc />
         public virtual async Task<TEntity> CreateAsync(TEntity entity)
         {
             foreach (var relationshipAttr in _targetedFields.Relationships)
             {
-                var trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, entity, out bool wasAlreadyTracked);
+                object trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, entity, out bool wasAlreadyTracked);
                 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)
-                {
                     /// 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();
 
+            // this ensures relationships get reloaded from the database if they have
+            // been requested. See https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/343
+            DetachRelationships(entity);
+
             return entity;
         }
 
@@ -169,7 +139,7 @@ private void LoadInverseRelationships(object trackedRelationshipValue, Relations
 
         private bool IsHasOneRelationship(string internalRelationshipName, Type type)
         {
-            var relationshipAttr = _resourceGraph.GetContextEntity(type).Relationships.SingleOrDefault(r => r.InternalRelationshipName == internalRelationshipName);
+            var relationshipAttr = _resourceGraph.GetContextEntity(type).Relationships.FirstOrDefault(r => r.InternalRelationshipName == internalRelationshipName);
             if (relationshipAttr != null)
             {
                 if (relationshipAttr is HasOneAttribute)
@@ -182,9 +152,7 @@ private bool IsHasOneRelationship(string internalRelationshipName, Type type)
             return !(type.GetProperty(internalRelationshipName).PropertyType.Inherits(typeof(IEnumerable)));
         }
 
-
-        /// <inheritdoc />
-        public void DetachRelationshipPointers(TEntity entity)
+        private void DetachRelationships(TEntity entity)
         {
             foreach (var relationshipAttr in _targetedFields.Relationships)
             {
@@ -212,22 +180,22 @@ public void DetachRelationshipPointers(TEntity entity)
         /// <inheritdoc />
         public virtual async Task<TEntity> UpdateAsync(TEntity updatedEntity)
         {
-            var databaseEntity = await GetAsync(updatedEntity.Id);
+            var databaseEntity = await Get(updatedEntity.Id).FirstOrDefaultAsync();
             if (databaseEntity == null)
                 return null;
 
-            foreach (var attr in _targetedFields.Attributes)
-                attr.SetValue(databaseEntity, attr.GetValue(updatedEntity));
+            foreach (var attribute in _targetedFields.Attributes)
+                attribute.SetValue(databaseEntity, attribute.GetValue(updatedEntity));
 
             foreach (var relationshipAttr in _targetedFields.Relationships)
             {
                 /// 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 currentlresy related 
+                /// trackedRelationshipValue is either equal to updatedPerson.todoItems,
+                /// or replaced with the same set (same ids) of todoItems from the EF Core change tracker,
+                /// which is the case if they were already tracked
+                object trackedRelationshipValue = GetTrackedRelationshipValue(relationshipAttr, updatedEntity, out _);
+                /// loads into the db context any persons currently related
                 /// to the todoItems in trackedRelationshipValue
                 LoadInverseRelationships(trackedRelationshipValue, relationshipAttr);
                 /// assigns the updated relationship to the database entity
@@ -238,7 +206,6 @@ public virtual async Task<TEntity> UpdateAsync(TEntity updatedEntity)
             return databaseEntity;
         }
 
-
         /// <summary>
         /// Responsible for getting the relationship value for a given relationship 
         /// attribute of a given entity. It ensures that the relationship value 
@@ -302,11 +269,10 @@ public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute
             await genericProcessor.UpdateRelationshipsAsync(parent, relationship, relationshipIds);
         }
 
-
         /// <inheritdoc />
         public virtual async Task<bool> DeleteAsync(TId id)
         {
-            var entity = await GetAsync(id);
+            var entity = await Get(id).FirstOrDefaultAsync();
             if (entity == null) return false;
             _dbSet.Remove(entity);
             await _context.SaveChangesAsync();
@@ -327,33 +293,6 @@ public virtual IQueryable<TEntity> Include(IQueryable<TEntity> entities, params
             return entities.Include(internalRelationshipPath);
         }
 
-        /// <inheritdoc />
-        public virtual IQueryable<TEntity> Include(IQueryable<TEntity> entities, string relationshipName)
-        {
-            if (string.IsNullOrWhiteSpace(relationshipName)) throw new JsonApiException(400, "Include parameter must not be empty if provided");
-
-            var relationshipChain = relationshipName.Split('.');
-
-            // variables mutated in recursive loop
-            // TODO: make recursive method
-            string internalRelationshipPath = null;
-            var entity = _currentRequest.GetRequestResource();
-            for (var i = 0; i < relationshipChain.Length; i++)
-            {
-                var requestedRelationship = relationshipChain[i];
-                var relationship = entity.Relationships.FirstOrDefault(r => r.PublicRelationshipName == requestedRelationship);
-
-                internalRelationshipPath = (internalRelationshipPath == null)
-                    ? relationship.RelationshipPath
-                    : $"{internalRelationshipPath}.{relationship.RelationshipPath}";
-
-                if (i < relationshipChain.Length)
-                    entity = _resourceGraph.GetContextEntity(relationship.Type);
-            }
-
-            return entities.Include(internalRelationshipPath);
-        }
-
         /// <inheritdoc />
         public virtual async Task<IEnumerable<TEntity>> PageAsync(IQueryable<TEntity> entities, int pageSize, int pageNumber)
         {
@@ -493,16 +432,23 @@ private IIdentifiable AttachOrGetTracked(IIdentifiable relationshipValue)
     }
 
     /// <inheritdoc />
-    public class DefaultEntityRepository<TEntity>
-        : DefaultEntityRepository<TEntity, int>,
-        IEntityRepository<TEntity>
+    public class DefaultEntityRepository<TEntity> : DefaultEntityRepository<TEntity, int>, IEntityRepository<TEntity>
         where TEntity : class, IIdentifiable<int>
     {
-        public DefaultEntityRepository(ICurrentRequest currentRequest, ITargetedFields updatedFields, IDbContextResolver contextResolver, IResourceGraph resourceGraph, IGenericProcessorFactory genericProcessorFactory, ResourceDefinition<TEntity> resourceDefinition = null) : base(currentRequest, updatedFields, contextResolver, resourceGraph, genericProcessorFactory, resourceDefinition)
+        public DefaultEntityRepository(ITargetedFields targetedFields,
+                                       IDbContextResolver contextResolver,
+                                       IResourceGraph resourceGraph,
+                                       IGenericProcessorFactory genericProcessorFactory)
+            : base(targetedFields, contextResolver, resourceGraph, genericProcessorFactory)
         {
         }
 
-        public DefaultEntityRepository(ICurrentRequest currentRequest, ITargetedFields updatedFields, IDbContextResolver contextResolver, IResourceGraph resourceGraph, IGenericProcessorFactory genericProcessorFactory, ResourceDefinition<TEntity> resourceDefinition = null, ILoggerFactory loggerFactory = null) : base(currentRequest, updatedFields, contextResolver, resourceGraph, genericProcessorFactory, resourceDefinition, loggerFactory)
+        public DefaultEntityRepository(ITargetedFields targetedFields,
+                                       IDbContextResolver contextResolver,
+                                       IResourceGraph resourceGraph,
+                                       IGenericProcessorFactory genericProcessorFactory,
+                                       ILoggerFactory loggerFactory = null)
+            : base(targetedFields, contextResolver, resourceGraph, genericProcessorFactory, loggerFactory)
         {
         }
     }

src/JsonApiDotNetCore/Data/IEntityReadRepository.cs

@@ -20,12 +20,14 @@ public interface IEntityReadRepository<TEntity, in TId>
         /// such as authorization of resources.
         /// </summary>
         IQueryable<TEntity> Get();
-
+        /// <summary>
+        /// Get the entity by id
+        /// </summary>
+        IQueryable<TEntity> Get(TId id);
         /// <summary>
         /// Apply fields to the provided queryable
         /// </summary>
         IQueryable<TEntity> Select(IQueryable<TEntity> entities, List<AttrAttribute> fields);
-
         /// <summary>
         /// Include a relationship in the query
         /// </summary>
@@ -35,51 +37,26 @@ public interface IEntityReadRepository<TEntity, in TId>
         /// </code>
         /// </example>
         IQueryable<TEntity> Include(IQueryable<TEntity> entities, params RelationshipAttribute[] inclusionChain);
-        [Obsolete]
-        IQueryable<TEntity> Include(IQueryable<TEntity> entities, string relationshipName);
-
         /// <summary>
         /// Apply a filter to the provided queryable
         /// </summary>
         IQueryable<TEntity> Filter(IQueryable<TEntity> entities, FilterQueryContext filterQuery);
-
         /// <summary>
         /// Apply a sort to the provided queryable
         /// </summary>
         IQueryable<TEntity> Sort(IQueryable<TEntity> entities, SortQueryContext sortQueries);
-
         /// <summary>
         /// Paginate the provided queryable
         /// </summary>
         Task<IEnumerable<TEntity>> PageAsync(IQueryable<TEntity> entities, int pageSize, int pageNumber);
-
-        /// <summary>
-        /// Get the entity by id
-        /// </summary>
-        Task<TEntity> GetAsync(TId id, List<AttrAttribute> fields = null);
-
-        /// <summary>
-        /// Get the entity with the specified id and include the relationship.
-        /// </summary>
-        /// <param name="id">The entity id</param>
-        /// <param name="relationship">The exposed relationship</param>
-        /// <example>
-        /// <code>
-        /// _todoItemsRepository.GetAndIncludeAsync(1, "achieved-date");
-        /// </code>
-        /// </example>
-        Task<TEntity> GetAndIncludeAsync(TId id, RelationshipAttribute relationship, List<AttrAttribute> fields = null);
-
         /// <summary>
         /// Count the total number of records
         /// </summary>
         Task<int> CountAsync(IQueryable<TEntity> entities);
-
         /// <summary>
         /// Get the first element in the collection, return the default value if collection is empty
         /// </summary>
         Task<TEntity> FirstOrDefaultAsync(IQueryable<TEntity> entities);
-
         /// <summary>
         /// Convert the collection to a materialized list
         /// </summary>