Fixed including extra resources from OnApplyIncudes (#1011)

* Refactored existing tests to use friendlier models

* Makes the serializer take changes from `IResourceDefinition.OnApplyChanges` into account. This enables adding extra includes from a resource definition.
This commit makes `ResponseResourceObjectBuilder` a bit faster too, because it no longer evaluates all constraints on each include.

* Improved hit tracking assertions for resource definition callbacks.
Optimization: Removed callback invocations for to-one include (see QueryLayerComposer) because their results are never used.

* For convenience, expose the resource context that matches TResource in JsonApiResourceDefinition.

Author: Bart Koelman

src/JsonApiDotNetCore/Configuration/JsonApiApplicationBuilder.cs

@@ -158,6 +158,7 @@ public void ConfigureServiceContainer(ICollection<Type> dbContextTypes)
             _services.AddScoped<IGenericServiceFactory, GenericServiceFactory>();
             _services.AddScoped(typeof(IResourceChangeTracker<>), typeof(ResourceChangeTracker<>));
             _services.AddScoped<IPaginationContext, PaginationContext>();
+            _services.AddScoped<IEvaluatedIncludeCache, EvaluatedIncludeCache>();
             _services.AddScoped<IQueryLayerComposer, QueryLayerComposer>();
             _services.AddScoped<IInverseNavigationResolver, InverseNavigationResolver>();
         }

src/JsonApiDotNetCore/Queries/Expressions/IncludeChainConverter.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using JsonApiDotNetCore.Resources.Annotations;
@@ -32,6 +33,11 @@ public IReadOnlyCollection<ResourceFieldChainExpression> GetRelationshipChains(I
         {
             ArgumentGuard.NotNull(include, nameof(include));
 
+            if (!include.Elements.Any())
+            {
+                return Array.Empty<ResourceFieldChainExpression>();
+            }
+
             var converter = new IncludeToChainsConverter();
             converter.Visit(include, null);
 

src/JsonApiDotNetCore/Queries/Internal/EvaluatedIncludeCache.cs

@@ -0,0 +1,22 @@
+using JsonApiDotNetCore.Queries.Expressions;
+
+namespace JsonApiDotNetCore.Queries.Internal
+{
+    /// <inheritdoc />
+    internal sealed class EvaluatedIncludeCache : IEvaluatedIncludeCache
+    {
+        private IncludeExpression _include;
+
+        /// <inheritdoc />
+        public void Set(IncludeExpression include)
+        {
+            _include = include;
+        }
+
+        /// <inheritdoc />
+        public IncludeExpression Get()
+        {
+            return _include;
+        }
+    }
+}

src/JsonApiDotNetCore/Queries/Internal/IEvaluatedIncludeCache.cs

@@ -0,0 +1,23 @@
+using JsonApiDotNetCore.Queries.Expressions;
+using JsonApiDotNetCore.Resources;
+
+namespace JsonApiDotNetCore.Queries.Internal
+{
+    /// <summary>
+    /// Provides in-memory storage for the evaluated inclusion tree within a request. This tree is produced from query string and resource definition
+    /// callbacks. The cache enables the serialization layer to take changes from <see cref="IResourceDefinition{TResource,TId}.OnApplyIncludes" /> into
+    /// account.
+    /// </summary>
+    public interface IEvaluatedIncludeCache
+    {
+        /// <summary>
+        /// Stores the evaluated inclusion tree for later usage.
+        /// </summary>
+        void Set(IncludeExpression include);
+
+        /// <summary>
+        /// Gets the evaluated inclusion tree that was stored earlier.
+        /// </summary>
+        IncludeExpression Get();
+    }
+}

src/JsonApiDotNetCore/Queries/Internal/QueryLayerComposer.cs

@@ -20,25 +20,28 @@ public class QueryLayerComposer : IQueryLayerComposer
         private readonly IJsonApiOptions _options;
         private readonly IPaginationContext _paginationContext;
         private readonly ITargetedFields _targetedFields;
+        private readonly IEvaluatedIncludeCache _evaluatedIncludeCache;
         private readonly SparseFieldSetCache _sparseFieldSetCache;
 
         public QueryLayerComposer(IEnumerable<IQueryConstraintProvider> constraintProviders, IResourceContextProvider resourceContextProvider,
             IResourceDefinitionAccessor resourceDefinitionAccessor, IJsonApiOptions options, IPaginationContext paginationContext,
-            ITargetedFields targetedFields)
+            ITargetedFields targetedFields, IEvaluatedIncludeCache evaluatedIncludeCache)
         {
             ArgumentGuard.NotNull(constraintProviders, nameof(constraintProviders));
             ArgumentGuard.NotNull(resourceContextProvider, nameof(resourceContextProvider));
             ArgumentGuard.NotNull(resourceDefinitionAccessor, nameof(resourceDefinitionAccessor));
             ArgumentGuard.NotNull(options, nameof(options));
             ArgumentGuard.NotNull(paginationContext, nameof(paginationContext));
             ArgumentGuard.NotNull(targetedFields, nameof(targetedFields));
+            ArgumentGuard.NotNull(evaluatedIncludeCache, nameof(evaluatedIncludeCache));
 
             _constraintProviders = constraintProviders;
             _resourceContextProvider = resourceContextProvider;
             _resourceDefinitionAccessor = resourceDefinitionAccessor;
             _options = options;
             _paginationContext = paginationContext;
             _targetedFields = targetedFields;
+            _evaluatedIncludeCache = evaluatedIncludeCache;
             _sparseFieldSetCache = new SparseFieldSetCache(_constraintProviders, resourceDefinitionAccessor);
         }
 
@@ -72,6 +75,8 @@ public QueryLayer ComposeFromConstraints(ResourceContext requestResource)
             QueryLayer topLayer = ComposeTopLayer(constraints, requestResource);
             topLayer.Include = ComposeChildren(topLayer, constraints);
 
+            _evaluatedIncludeCache.Set(topLayer.Include);
+
             return topLayer;
         }
 
@@ -159,12 +164,15 @@ private IReadOnlyCollection<IncludeElementExpression> ProcessIncludeSet(IReadOnl
                     // @formatter:wrap_chained_method_calls restore
 
                     ResourceContext resourceContext = _resourceContextProvider.GetResourceContext(includeElement.Relationship.RightType);
+                    bool isToManyRelationship = includeElement.Relationship is HasManyAttribute;
 
                     var child = new QueryLayer(resourceContext)
                     {
-                        Filter = GetFilter(expressionsInCurrentScope, resourceContext),
-                        Sort = GetSort(expressionsInCurrentScope, resourceContext),
-                        Pagination = ((JsonApiOptions)_options).DisableChildrenPagination ? null : GetPagination(expressionsInCurrentScope, resourceContext),
+                        Filter = isToManyRelationship ? GetFilter(expressionsInCurrentScope, resourceContext) : null,
+                        Sort = isToManyRelationship ? GetSort(expressionsInCurrentScope, resourceContext) : null,
+                        Pagination = isToManyRelationship
+                            ? ((JsonApiOptions)_options).DisableChildrenPagination ? null : GetPagination(expressionsInCurrentScope, resourceContext)
+                            : null,
                         Projection = GetProjectionForSparseAttributeSet(resourceContext)
                     };
 

src/JsonApiDotNetCore/QueryStrings/Internal/ResourceDefinitionQueryableParameterReader.cs

@@ -56,7 +56,7 @@ public virtual void Read(string parameterName, StringValues parameterValue)
 
         private object GetQueryableHandler(string parameterName)
         {
-            Type resourceType = _request.PrimaryResource.ResourceType;
+            Type resourceType = (_request.SecondaryResource ?? _request.PrimaryResource).ResourceType;
             object handler = _resourceDefinitionAccessor.GetQueryableHandlerForQueryStringParameter(resourceType, parameterName);
 
             if (handler != null && _request.Kind != EndpointKind.Primary)

src/JsonApiDotNetCore/Resources/JsonApiResourceDefinition.cs

@@ -37,11 +37,17 @@ public class JsonApiResourceDefinition<TResource, TId> : IResourceDefinition<TRe
     {
         protected IResourceGraph ResourceGraph { get; }
 
+        /// <summary>
+        /// Provides metadata for the resource type <typeparamref name="TResource" />.
+        /// </summary>
+        protected ResourceContext ResourceContext { get; }
+
         public JsonApiResourceDefinition(IResourceGraph resourceGraph)
         {
             ArgumentGuard.NotNull(resourceGraph, nameof(resourceGraph));
 
             ResourceGraph = resourceGraph;
+            ResourceContext = resourceGraph.GetResourceContext<TResource>();
         }
 
         /// <inheritdoc />

src/JsonApiDotNetCore/Serialization/AtomicOperationsResponseSerializer.cs

@@ -4,6 +4,7 @@
 using JetBrains.Annotations;
 using JsonApiDotNetCore.Configuration;
 using JsonApiDotNetCore.Middleware;
+using JsonApiDotNetCore.Queries.Internal;
 using JsonApiDotNetCore.Resources;
 using JsonApiDotNetCore.Resources.Annotations;
 using JsonApiDotNetCore.Serialization.Building;
@@ -21,27 +22,31 @@ public sealed class AtomicOperationsResponseSerializer : BaseSerializer, IJsonAp
         private readonly ILinkBuilder _linkBuilder;
         private readonly IFieldsToSerialize _fieldsToSerialize;
         private readonly IResourceDefinitionAccessor _resourceDefinitionAccessor;
+        private readonly IEvaluatedIncludeCache _evaluatedIncludeCache;
         private readonly IJsonApiRequest _request;
         private readonly IJsonApiOptions _options;
 
         /// <inheritdoc />
         public string ContentType { get; } = HeaderConstants.AtomicOperationsMediaType;
 
         public AtomicOperationsResponseSerializer(IResourceObjectBuilder resourceObjectBuilder, IMetaBuilder metaBuilder, ILinkBuilder linkBuilder,
-            IFieldsToSerialize fieldsToSerialize, IResourceDefinitionAccessor resourceDefinitionAccessor, IJsonApiRequest request, IJsonApiOptions options)
+            IFieldsToSerialize fieldsToSerialize, IResourceDefinitionAccessor resourceDefinitionAccessor, IEvaluatedIncludeCache evaluatedIncludeCache,
+            IJsonApiRequest request, IJsonApiOptions options)
             : base(resourceObjectBuilder)
         {
             ArgumentGuard.NotNull(metaBuilder, nameof(metaBuilder));
             ArgumentGuard.NotNull(linkBuilder, nameof(linkBuilder));
             ArgumentGuard.NotNull(fieldsToSerialize, nameof(fieldsToSerialize));
             ArgumentGuard.NotNull(resourceDefinitionAccessor, nameof(resourceDefinitionAccessor));
+            ArgumentGuard.NotNull(evaluatedIncludeCache, nameof(evaluatedIncludeCache));
             ArgumentGuard.NotNull(request, nameof(request));
             ArgumentGuard.NotNull(options, nameof(options));
 
             _metaBuilder = metaBuilder;
             _linkBuilder = linkBuilder;
             _fieldsToSerialize = fieldsToSerialize;
             _resourceDefinitionAccessor = resourceDefinitionAccessor;
+            _evaluatedIncludeCache = evaluatedIncludeCache;
             _request = request;
             _options = options;
         }
@@ -93,6 +98,7 @@ private AtomicResultObject SerializeOperation(OperationContainer operation)
             {
                 _request.CopyFrom(operation.Request);
                 _fieldsToSerialize.ResetCache();
+                _evaluatedIncludeCache.Set(null);
 
                 _resourceDefinitionAccessor.OnSerialize(operation.Resource);
 

src/JsonApiDotNetCore/Serialization/Building/ResponseResourceObjectBuilder.cs

@@ -5,7 +5,6 @@
 using JsonApiDotNetCore.Queries;
 using JsonApiDotNetCore.Queries.Expressions;
 using JsonApiDotNetCore.Queries.Internal;
-using JsonApiDotNetCore.QueryStrings;
 using JsonApiDotNetCore.Resources;
 using JsonApiDotNetCore.Resources.Annotations;
 using JsonApiDotNetCore.Serialization.Objects;
@@ -17,28 +16,31 @@ public class ResponseResourceObjectBuilder : ResourceObjectBuilder
     {
         private static readonly IncludeChainConverter IncludeChainConverter = new IncludeChainConverter();
 
+        private readonly ILinkBuilder _linkBuilder;
         private readonly IIncludedResourceObjectBuilder _includedBuilder;
-        private readonly IEnumerable<IQueryConstraintProvider> _constraintProviders;
         private readonly IResourceDefinitionAccessor _resourceDefinitionAccessor;
-        private readonly ILinkBuilder _linkBuilder;
+        private readonly IEvaluatedIncludeCache _evaluatedIncludeCache;
         private readonly SparseFieldSetCache _sparseFieldSetCache;
+
         private RelationshipAttribute _requestRelationship;
 
         public ResponseResourceObjectBuilder(ILinkBuilder linkBuilder, IIncludedResourceObjectBuilder includedBuilder,
             IEnumerable<IQueryConstraintProvider> constraintProviders, IResourceContextProvider resourceContextProvider,
-            IResourceDefinitionAccessor resourceDefinitionAccessor, IResourceObjectBuilderSettingsProvider settingsProvider)
+            IResourceDefinitionAccessor resourceDefinitionAccessor, IResourceObjectBuilderSettingsProvider settingsProvider,
+            IEvaluatedIncludeCache evaluatedIncludeCache)
             : base(resourceContextProvider, settingsProvider.Get())
         {
             ArgumentGuard.NotNull(linkBuilder, nameof(linkBuilder));
             ArgumentGuard.NotNull(includedBuilder, nameof(includedBuilder));
             ArgumentGuard.NotNull(constraintProviders, nameof(constraintProviders));
             ArgumentGuard.NotNull(resourceDefinitionAccessor, nameof(resourceDefinitionAccessor));
+            ArgumentGuard.NotNull(evaluatedIncludeCache, nameof(evaluatedIncludeCache));
 
             _linkBuilder = linkBuilder;
             _includedBuilder = includedBuilder;
-            _constraintProviders = constraintProviders;
             _resourceDefinitionAccessor = resourceDefinitionAccessor;
-            _sparseFieldSetCache = new SparseFieldSetCache(_constraintProviders, resourceDefinitionAccessor);
+            _evaluatedIncludeCache = evaluatedIncludeCache;
+            _sparseFieldSetCache = new SparseFieldSetCache(constraintProviders, resourceDefinitionAccessor);
         }
 
         public RelationshipEntry Build(IIdentifiable resource, RelationshipAttribute requestRelationship)
@@ -72,7 +74,7 @@ protected override RelationshipEntry GetRelationshipData(RelationshipAttribute r
             ArgumentGuard.NotNull(resource, nameof(resource));
 
             RelationshipEntry relationshipEntry = null;
-            IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> relationshipChains = GetInclusionChain(relationship);
+            IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> relationshipChains = GetInclusionChainsStartingWith(relationship);
 
             if (Equals(relationship, _requestRelationship) || relationshipChains.Any())
             {
@@ -116,35 +118,24 @@ private bool IsRelationshipInSparseFieldSet(RelationshipAttribute relationship)
         }
 
         /// <summary>
-        /// Inspects the included relationship chains (see <see cref="IIncludeQueryStringParameterReader" /> to see if <paramref name="relationship" /> should be
-        /// included or not.
+        /// Inspects the included relationship chains and selects the ones that starts with the specified relationship.
         /// </summary>
-        private IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> GetInclusionChain(RelationshipAttribute relationship)
+        private IReadOnlyCollection<IReadOnlyCollection<RelationshipAttribute>> GetInclusionChainsStartingWith(RelationshipAttribute relationship)
         {
-            // @formatter:wrap_chained_method_calls chop_always
-            // @formatter:keep_existing_linebreaks true
-
-            ResourceFieldChainExpression[] chains = _constraintProviders
-                .SelectMany(provider => provider.GetConstraints())
-                .Select(expressionInScope => expressionInScope.Expression)
-                .OfType<IncludeExpression>()
-                .SelectMany(IncludeChainConverter.GetRelationshipChains)
-                .ToArray();
-
-            // @formatter:keep_existing_linebreaks restore
-            // @formatter:wrap_chained_method_calls restore
+            IncludeExpression include = _evaluatedIncludeCache.Get() ?? IncludeExpression.Empty;
+            IReadOnlyCollection<ResourceFieldChainExpression> chains = IncludeChainConverter.GetRelationshipChains(include);
 
-            var inclusionChain = new List<IReadOnlyCollection<RelationshipAttribute>>();
+            var inclusionChains = new List<IReadOnlyCollection<RelationshipAttribute>>();
 
             foreach (ResourceFieldChainExpression chain in chains)
             {
                 if (chain.Fields.First().Equals(relationship))
                 {
-                    inclusionChain.Add(chain.Fields.Cast<RelationshipAttribute>().ToArray());
+                    inclusionChains.Add(chain.Fields.Cast<RelationshipAttribute>().ToArray());
                 }
             }
 
-            return inclusionChain;
+            return inclusionChains;
         }
     }
 }

test/JsonApiDotNetCoreExampleTests/IntegrationTests/Archiving/TelevisionBroadcastDefinition.cs

@@ -24,7 +24,6 @@ public sealed class TelevisionBroadcastDefinition : JsonApiResourceDefinition<Te
         private readonly TelevisionDbContext _dbContext;
         private readonly IJsonApiRequest _request;
         private readonly IEnumerable<IQueryConstraintProvider> _constraintProviders;
-        private readonly ResourceContext _broadcastContext;
 
         private DateTimeOffset? _storedArchivedAt;
 
@@ -35,7 +34,6 @@ public TelevisionBroadcastDefinition(IResourceGraph resourceGraph, TelevisionDbC
             _dbContext = dbContext;
             _request = request;
             _constraintProviders = constraintProviders;
-            _broadcastContext = resourceGraph.GetResourceContext<TelevisionBroadcast>();
         }
 
         public override FilterExpression OnApplyFilter(FilterExpression existingFilter)
@@ -46,8 +44,7 @@ public override FilterExpression OnApplyFilter(FilterExpression existingFilter)
 
                 if (IsReturningCollectionOfTelevisionBroadcasts() && !HasFilterOnArchivedAt(existingFilter))
                 {
-                    AttrAttribute archivedAtAttribute =
-                        _broadcastContext.Attributes.Single(attr => attr.Property.Name == nameof(TelevisionBroadcast.ArchivedAt));
+                    AttrAttribute archivedAtAttribute = ResourceContext.Attributes.Single(attr => attr.Property.Name == nameof(TelevisionBroadcast.ArchivedAt));
 
                     var archivedAtChain = new ResourceFieldChainExpression(archivedAtAttribute);
 
@@ -71,7 +68,7 @@ private bool IsRequestingCollectionOfTelevisionBroadcasts()
         {
             if (_request.IsCollection)
             {
-                if (_request.PrimaryResource == _broadcastContext || _request.SecondaryResource == _broadcastContext)
+                if (_request.PrimaryResource == ResourceContext || _request.SecondaryResource == ResourceContext)
                 {
                     return true;
                 }
@@ -97,7 +94,7 @@ private bool IsIncludingCollectionOfTelevisionBroadcasts()
 
             foreach (IncludeElementExpression includeElement in includeElements)
             {
-                if (includeElement.Relationship is HasManyAttribute && includeElement.Relationship.RightType == _broadcastContext.ResourceType)
+                if (includeElement.Relationship is HasManyAttribute && includeElement.Relationship.RightType == ResourceContext.ResourceType)
                 {
                     return true;
                 }