Fixed: relationships were not returned when using sparse fieldsets

Fixed: filter out duplicate selections
Fixed: Top-level fields were missing when selecting only related fields
Fixed: Select all fields when sparse fieldset contains a read-only property

Author: Bart Koelman

src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs

@@ -62,14 +62,11 @@ public virtual IQueryable<TResource> Get(TId id)
         }
 
         /// <inheritdoc />
-        public virtual IQueryable<TResource> Select(IQueryable<TResource> entities, IEnumerable<AttrAttribute> fields = null)
+        public virtual IQueryable<TResource> Select(IQueryable<TResource> entities, IEnumerable<string> propertyNames = null)
         {
-            _logger.LogTrace($"Entering {nameof(Select)}({nameof(entities)}, {nameof(fields)}).");
+            _logger.LogTrace($"Entering {nameof(Select)}({nameof(entities)}, {nameof(propertyNames)}).");
 
-            if (fields != null && fields.Any())
-                return entities.Select(fields);
-
-            return entities;
+            return entities.Select(propertyNames);
         }
 
         /// <inheritdoc />

src/JsonApiDotNetCore/Data/IResourceReadRepository.cs

@@ -26,7 +26,7 @@ public interface IResourceReadRepository<TResource, in TId>
         /// <summary>
         /// Apply fields to the provided queryable
         /// </summary>
-        IQueryable<TResource> Select(IQueryable<TResource> entities, IEnumerable<AttrAttribute> fields);
+        IQueryable<TResource> Select(IQueryable<TResource> entities, IEnumerable<string> propertyNames);
         /// <summary>
         /// Include a relationship in the query
         /// </summary>

src/JsonApiDotNetCore/Extensions/IQueryableExtensions.cs

@@ -64,8 +64,10 @@ public static IQueryable<TSource> Filter<TSource>(this IQueryable<TSource> sourc
             return CallGenericWhereMethod(source, filterQuery);
         }
 
-        public static IQueryable<TSource> Select<TSource>(this IQueryable<TSource> source, IEnumerable<AttrAttribute> columns)
-            => CallGenericSelectMethod(source, columns.Select(attr => attr.PropertyInfo.Name).ToList());
+        public static IQueryable<TSource> Select<TSource>(this IQueryable<TSource> source, IEnumerable<string> columns)
+        {
+            return columns == null || !columns.Any() ? source : CallGenericSelectMethod(source, columns);
+        }
 
         public static IOrderedQueryable<TSource> Sort<TSource>(this IQueryable<TSource> source, SortQueryContext sortQuery)
         {
@@ -339,26 +341,28 @@ private static Expression CreateTupleAccessForConstantExpression(object value, T
             return Expression.Property(tupleCreateCall, "Item1");
         }
 
-        private static IQueryable<TSource> CallGenericSelectMethod<TSource>(IQueryable<TSource> source, List<string> columns)
+        private static IQueryable<TSource> CallGenericSelectMethod<TSource>(IQueryable<TSource> source, IEnumerable<string> columns)
         {
             var sourceType = typeof(TSource);
             var parameter = Expression.Parameter(source.ElementType, "x");
-            var sourceProperties = new List<string>();
+            var sourceProperties = new HashSet<string>();
 
             // Store all property names to it's own related property (name as key)
-            var nestedTypesAndProperties = new Dictionary<string, List<string>>();
+            var nestedTypesAndProperties = new Dictionary<string, HashSet<string>>();
             foreach (var column in columns)
             {
                 var props = column.Split('.');
                 if (props.Length > 1) // Nested property
                 {
                     if (nestedTypesAndProperties.TryGetValue(props[0], out var properties) == false)
-                        nestedTypesAndProperties.Add(props[0], new List<string> { nameof(Identifiable.Id), props[1] });
+                        nestedTypesAndProperties.Add(props[0], new HashSet<string> { nameof(Identifiable.Id), props[1] });
                     else
                         properties.Add(props[1]);
                 }
                 else
+                {
                     sourceProperties.Add(props[0]);
+                }
             }
 
             // Bind attributes on TSource

src/JsonApiDotNetCore/QueryParameterServices/Contracts/ISparseFieldsService.cs

@@ -1,4 +1,4 @@
-using System.Collections.Generic;
+using System.Collections.Generic;
 using JsonApiDotNetCore.Models;
 
 namespace JsonApiDotNetCore.Query
@@ -12,7 +12,11 @@ public interface ISparseFieldsService : IQueryParameterService
         /// Gets the list of targeted fields. If a relationship is supplied,
         /// gets the list of targeted fields for that relationship.
         /// </summary>
-        /// <param name="relationship"></param>
         List<AttrAttribute> Get(RelationshipAttribute relationship = null);
+
+        /// <summary>
+        /// Gets the set of all targeted fields, including fields for related entities, as a set of dotted property names.
+        /// </summary>
+        ISet<string> GetAll();
     }
-}
+}

src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs

@@ -17,16 +17,16 @@ public class SparseFieldsService : QueryParameterService, ISparseFieldsService
         /// <summary>
         /// The selected fields for the primary resource of this request.
         /// </summary>
-        private List<AttrAttribute> _selectedFields;
+        private readonly List<AttrAttribute> _selectedFields = new List<AttrAttribute>();
+
         /// <summary>
         /// The selected field for any included relationships
         /// </summary>
-        private readonly Dictionary<RelationshipAttribute, List<AttrAttribute>> _selectedRelationshipFields;
+        private readonly Dictionary<RelationshipAttribute, List<AttrAttribute>> _selectedRelationshipFields = new Dictionary<RelationshipAttribute, List<AttrAttribute>>();
 
-        public SparseFieldsService(IResourceGraph resourceGraph, ICurrentRequest currentRequest) : base(resourceGraph, currentRequest)
+        public SparseFieldsService(IResourceGraph resourceGraph, ICurrentRequest currentRequest) 
+            : base(resourceGraph, currentRequest)
         {
-            _selectedFields = new List<AttrAttribute>();
-            _selectedRelationshipFields = new Dictionary<RelationshipAttribute, List<AttrAttribute>>();
         }
 
         /// <inheritdoc/>
@@ -35,8 +35,23 @@ public List<AttrAttribute> Get(RelationshipAttribute relationship = null)
             if (relationship == null)
                 return _selectedFields;
 
-            _selectedRelationshipFields.TryGetValue(relationship, out var fields);
-            return fields;
+            return _selectedRelationshipFields.TryGetValue(relationship, out var fields)
+                ? fields
+                : new List<AttrAttribute>();
+        }
+
+        public ISet<string> GetAll()
+        {
+            var properties = new HashSet<string>();
+            properties.AddRange(_selectedFields.Select(x => x.PropertyInfo.Name));
+
+            foreach (var pair in _selectedRelationshipFields)
+            {
+                string pathPrefix = pair.Key.RelationshipPath + ".";
+                properties.AddRange(pair.Value.Select(x => pathPrefix + x.PropertyInfo.Name));
+            }
+
+            return properties;
         }
 
         /// <inheritdoc/>
@@ -67,9 +82,9 @@ public virtual void Parse(string parameterName, StringValues parameterValue)
             var keySplit = parameterName.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET);
 
             if (keySplit.Length == 1)
-            {   // input format: fields=prop1,prop2
-                foreach (var field in fields)
-                    RegisterRequestResourceField(field, parameterName);
+            {
+                // input format: fields=prop1,prop2
+                RegisterRequestResourceFields(fields, parameterName);
             }
             else
             {  // input format: fields[articles]=prop1,prop2
@@ -98,45 +113,73 @@ public virtual void Parse(string parameterName, StringValues parameterValue)
                         $"'{navigation}' in 'fields[{navigation}]' is not a valid relationship of {_requestResource.ResourceName}.");
                 }
 
-                foreach (var field in fields)
-                    RegisterRelatedResourceField(field, relationship, parameterName);
+                RegisterRelatedResourceFields(fields, relationship, parameterName);
             }
         }
 
         /// <summary>
-        /// Registers field selection queries of the form articles?fields[author]=firstName
+        /// Registers field selection of the form articles?fields[author]=firstName,lastName
         /// </summary>
-        private void RegisterRelatedResourceField(string field, RelationshipAttribute relationship, string parameterName)
+        private void RegisterRelatedResourceFields(IEnumerable<string> fields, RelationshipAttribute relationship, string parameterName)
         {
-            var relationProperty = _resourceGraph.GetResourceContext(relationship.RightType);
-            var attr = relationProperty.Attributes.SingleOrDefault(a => a.Is(field));
-            if (attr == null)
+            var selectedFields = new List<AttrAttribute>();
+
+            foreach (var field in fields)
             {
-                // TODO: Add unit test for this error, once the nesting limitation is removed and this code becomes reachable again.
+                var relationProperty = _resourceGraph.GetResourceContext(relationship.RightType);
+                var attr = relationProperty.Attributes.SingleOrDefault(a => a.Is(field));
+                if (attr == null)
+                {
+                    // TODO: Add unit test for this error, once the nesting limitation is removed and this code becomes reachable again.
+
+                    throw new InvalidQueryStringParameterException(parameterName,
+                        "Sparse field navigation path refers to an invalid related field.",
+                        $"Related resource '{relationship.RightType.Name}' does not contain an attribute named '{field}'.");
+                }
 
-                throw new InvalidQueryStringParameterException(parameterName, "Sparse field navigation path refers to an invalid related field.",
-                    $"Related resource '{relationship.RightType.Name}' does not contain an attribute named '{field}'.");
+                if (attr.PropertyInfo.SetMethod == null)
+                {
+                    // A read-only property was selected. Its value likely depends on another property, so include all related fields.
+                    return;
+                }
+
+                selectedFields.Add(attr);
             }
 
             if (!_selectedRelationshipFields.TryGetValue(relationship, out var registeredFields))
+            {
                 _selectedRelationshipFields.Add(relationship, registeredFields = new List<AttrAttribute>());
-            registeredFields.Add(attr);
+            }
+            registeredFields.AddRange(selectedFields);
         }
 
         /// <summary>
-        /// Registers field selection queries of the form articles?fields=title
+        /// Registers field selection of the form articles?fields=title,description
         /// </summary>
-        private void RegisterRequestResourceField(string field, string parameterName)
+        private void RegisterRequestResourceFields(IEnumerable<string> fields, string parameterName)
         {
-            var attr = _requestResource.Attributes.SingleOrDefault(a => a.Is(field));
-            if (attr == null)
+            var selectedFields = new List<AttrAttribute>();
+
+            foreach (var field in fields)
             {
-                throw new InvalidQueryStringParameterException(parameterName,
-                    "The specified field does not exist on the requested resource.",
-                    $"The field '{field}' does not exist on resource '{_requestResource.ResourceName}'.");
+                var attr = _requestResource.Attributes.SingleOrDefault(a => a.Is(field));
+                if (attr == null)
+                {
+                    throw new InvalidQueryStringParameterException(parameterName,
+                        "The specified field does not exist on the requested resource.",
+                        $"The field '{field}' does not exist on resource '{_requestResource.ResourceName}'.");
+                }
+
+                if (attr.PropertyInfo.SetMethod == null)
+                {
+                    // A read-only property was selected. Its value likely depends on another property, so include all resource fields.
+                    return;
+                }
+
+                selectedFields.Add(attr);
             }
 
-            (_selectedFields ??= new List<AttrAttribute>()).Add(attr);
+            _selectedFields.AddRange(selectedFields);
         }
     }
 }

src/JsonApiDotNetCore/Serialization/Server/FieldsToSerialize.cs

@@ -34,7 +34,7 @@ public List<AttrAttribute> GetAllowedAttributes(Type type, RelationshipAttribute
                 allowed = allowed.Intersect(resourceDefinition.GetAllowedAttributes()).ToList();
 
             var sparseFieldsSelection = _sparseFieldsService.Get(relationship);
-            if (sparseFieldsSelection != null && sparseFieldsSelection.Any())
+            if (sparseFieldsSelection.Any())
                 // from the allowed attributes, select the ones flagged by sparse field selection.
                 allowed = allowed.Intersect(sparseFieldsSelection).ToList();
 

src/JsonApiDotNetCore/Services/DefaultResourceService.cs

@@ -159,7 +159,7 @@ public virtual async Task<TResource> GetRelationshipsAsync(TId id, string relati
             // BeforeRead hook execution
             _hookExecutor?.BeforeRead<TResource>(ResourcePipeline.GetRelationship, id.ToString());
 
-            var entityQuery = ApplyInclude(_repository.Get(id), chainPrefix: new List<RelationshipAttribute> { relationship });
+            var entityQuery = ApplyInclude(_repository.Get(id), relationship);
             var entity = await _repository.FirstOrDefaultAsync(entityQuery);
 
             if (entity == null)
@@ -299,78 +299,92 @@ protected virtual IQueryable<TResource> ApplyFilter(IQueryable<TResource> entiti
             return entities;
         }
 
-
         /// <summary>
         /// Applies include queries
         /// </summary>
-        protected virtual IQueryable<TResource> ApplyInclude(IQueryable<TResource> entities, IEnumerable<RelationshipAttribute> chainPrefix = null)
+        protected virtual IQueryable<TResource> ApplyInclude(IQueryable<TResource> entities, RelationshipAttribute chainPrefix = null)
         {
             var chains = _includeService.Get();
-            bool hasInclusionChain = chains.Any();
 
-            if (chains == null)
+            if (chainPrefix != null)
             {
-                throw new Exception();
+                chains.Add(new List<RelationshipAttribute>());
             }
 
-            if (chainPrefix != null && !hasInclusionChain)
-            {
-               hasInclusionChain = true;
-               chains.Add(new List<RelationshipAttribute>());
-            }
-
-
-            if (hasInclusionChain)
+            foreach (var inclusionChain in chains)
             {
-                foreach (var inclusionChain in chains)
+                if (chainPrefix != null)
                 {
-                    if (chainPrefix != null)
-                    {
-                        inclusionChain.InsertRange(0, chainPrefix);
-                    }
-                    entities = _repository.Include(entities, inclusionChain.ToArray());
+                    inclusionChain.Insert(0, chainPrefix);
                 }
+
+                entities = _repository.Include(entities, inclusionChain);
             }
 
             return entities;
         }
 
         /// <summary>
-        /// Applies sparse field selection queries
+        /// Applies sparse field selection to queries
         /// </summary>
-        /// <param name="entities"></param>
-        /// <returns></returns>
         protected virtual IQueryable<TResource> ApplySelect(IQueryable<TResource> entities)
         {
-            var fields = _sparseFieldsService.Get();
-            if (fields != null && fields.Any())
-                entities = _repository.Select(entities, fields.ToArray());
+            var propertyNames = _sparseFieldsService.GetAll();
+
+            if (propertyNames.Any())
+            {
+                // All resources without a sparse fieldset specified must be entirely selected.
+                EnsureResourcesWithoutSparseFieldSetAreAddedToSelect(propertyNames);
+            }
 
+            entities = _repository.Select(entities, propertyNames);
             return entities;
         }
 
+        private void EnsureResourcesWithoutSparseFieldSetAreAddedToSelect(ISet<string> propertyNames)
+        {
+            bool hasTopLevelSparseFieldSet = propertyNames.Any(x => !x.Contains("."));
+            if (!hasTopLevelSparseFieldSet)
+            {
+                var topPropertyNames = _currentRequestResource.Attributes
+                    .Where(x => x.PropertyInfo.SetMethod != null)
+                    .Select(x => x.PropertyInfo.Name);
+                propertyNames.AddRange(topPropertyNames);
+            }
+
+            var chains = _includeService.Get();
+            foreach (var inclusionChain in chains)
+            {
+                string relationshipPath = null;
+                foreach (var relationship in inclusionChain)
+                {
+                    relationshipPath = relationshipPath == null
+                        ? relationship.RelationshipPath
+                        : $"{relationshipPath}.{relationship.RelationshipPath}";
+                }
+
+                if (relationshipPath != null)
+                {
+                    bool hasRelationSparseFieldSet = propertyNames.Any(x => x.StartsWith(relationshipPath + "."));
+                    if (!hasRelationSparseFieldSet)
+                    {
+                        propertyNames.Add(relationshipPath);
+                    }
+                }
+            }
+        }
+
         /// <summary>
         /// Get the specified id with relationships provided in the post request
         /// </summary>
-        /// <param name="id"></param>i
-        /// <returns></returns>
         private async Task<TResource> GetWithRelationshipsAsync(TId id)
         {
-            var sparseFieldset = _sparseFieldsService.Get();
-            var query = _repository.Select(_repository.Get(id), sparseFieldset.ToArray());
+            var query = _repository.Get(id);
+            query = ApplyInclude(query);
+            query = ApplySelect(query);
 
-            foreach (var chain in _includeService.Get())
-                query = _repository.Include(query, chain.ToArray());
-
-            TResource value;
-            // https://github.com/aspnet/EntityFrameworkCore/issues/6573
-            if (sparseFieldset.Any())
-                value = query.FirstOrDefault();
-            else
-                value = await _repository.FirstOrDefaultAsync(query);
-
-
-            return value;
+            var entity = await _repository.FirstOrDefaultAsync(query);
+            return entity;
         }
 
         private bool IsNull(params object[] values)