Allow derived inclusion chains in filter/sort/page query strings (#1756)

* Simplify IQueryStringParameterScopeParser

* Expose source text on parser base class

* Move parsing of include chains into base class

* Allow derived types in the scope of sort/filter/page query string parameters

Author: bkoelman

src/JsonApiDotNetCore/Queries/Expressions/IncludeChainConverter.cs

@@ -8,6 +8,12 @@ namespace JsonApiDotNetCore.Queries.Expressions;
 /// </summary>
 internal sealed class IncludeChainConverter
 {
+    public static IncludeChainConverter Instance { get; } = new();
+
+    private IncludeChainConverter()
+    {
+    }
+
     /// <summary>
     /// Converts a tree of inclusions into a set of relationship chains.
     /// </summary>

src/JsonApiDotNetCore/Queries/Expressions/IncludeExpression.cs

@@ -13,8 +13,6 @@ namespace JsonApiDotNetCore.Queries.Expressions;
 [PublicAPI]
 public class IncludeExpression : QueryExpression
 {
-    private static readonly IncludeChainConverter IncludeChainConverter = new();
-
     public static readonly IncludeExpression Empty = new();
 
     /// <summary>
@@ -51,7 +49,7 @@ public override string ToFullString()
 
     private string InnerToString(bool toFullString)
     {
-        IReadOnlyCollection<ResourceFieldChainExpression> chains = IncludeChainConverter.GetRelationshipChains(this);
+        IReadOnlyCollection<ResourceFieldChainExpression> chains = IncludeChainConverter.Instance.GetRelationshipChains(this);
         return string.Join(",", chains.Select(field => toFullString ? field.ToFullString() : field.ToString()).Distinct().OrderBy(name => name));
     }
 

src/JsonApiDotNetCore/Queries/Expressions/PaginationElementQueryStringValueExpression.cs

@@ -10,12 +10,12 @@ namespace JsonApiDotNetCore.Queries.Expressions;
 /// .
 /// </summary>
 [PublicAPI]
-public class PaginationElementQueryStringValueExpression(ResourceFieldChainExpression? scope, int value, int position) : QueryExpression
+public class PaginationElementQueryStringValueExpression(IncludeExpression? scope, int value, int position) : QueryExpression
 {
     /// <summary>
-    /// The relationship this pagination applies to. Chain format: zero or more relationships, followed by a to-many relationship.
+    /// The relationship this pagination applies to. Format: zero or more relationships, followed by a to-many relationship.
     /// </summary>
-    public ResourceFieldChainExpression? Scope { get; } = scope;
+    public IncludeExpression? Scope { get; } = scope;
 
     /// <summary>
     /// The numeric pagination value.

src/JsonApiDotNetCore/Queries/Expressions/QueryExpressionRewriter.cs

@@ -205,7 +205,7 @@ public override QueryExpression VisitSparseFieldSet(SparseFieldSetExpression exp
     public override QueryExpression? VisitQueryStringParameterScope(QueryStringParameterScopeExpression expression, TArgument argument)
     {
         var newParameterName = Visit(expression.ParameterName, argument) as LiteralConstantExpression;
-        ResourceFieldChainExpression? newScope = expression.Scope != null ? Visit(expression.Scope, argument) as ResourceFieldChainExpression : null;
+        IncludeExpression? newScope = expression.Scope != null ? Visit(expression.Scope, argument) as IncludeExpression : null;
 
         if (newParameterName != null)
         {
@@ -226,7 +226,7 @@ public override QueryExpression VisitPaginationQueryStringValue(PaginationQueryS
 
     public override QueryExpression VisitPaginationElementQueryStringValue(PaginationElementQueryStringValueExpression expression, TArgument argument)
     {
-        ResourceFieldChainExpression? newScope = expression.Scope != null ? Visit(expression.Scope, argument) as ResourceFieldChainExpression : null;
+        IncludeExpression? newScope = expression.Scope != null ? Visit(expression.Scope, argument) as IncludeExpression : null;
 
         var newExpression = new PaginationElementQueryStringValueExpression(newScope, expression.Value, expression.Position);
         return newExpression.Equals(expression) ? expression : newExpression;

src/JsonApiDotNetCore/Queries/Expressions/QueryStringParameterScopeExpression.cs

@@ -25,9 +25,9 @@ public class QueryStringParameterScopeExpression : QueryExpression
     /// The scope this parameter value applies to, or <c>null</c> for the URL endpoint scope. Chain format for the filter/sort parameters: an optional list
     /// of relationships, followed by a to-many relationship.
     /// </summary>
-    public ResourceFieldChainExpression? Scope { get; }
+    public IncludeExpression? Scope { get; }
 
-    public QueryStringParameterScopeExpression(LiteralConstantExpression parameterName, ResourceFieldChainExpression? scope)
+    public QueryStringParameterScopeExpression(LiteralConstantExpression parameterName, IncludeExpression? scope)
     {
         ArgumentNullException.ThrowIfNull(parameterName);
 

src/JsonApiDotNetCore/Queries/Parsing/IQueryStringParameterScopeParser.cs

@@ -1,12 +1,11 @@
 using JsonApiDotNetCore.Configuration;
 using JsonApiDotNetCore.Queries.Expressions;
-using JsonApiDotNetCore.QueryStrings.FieldChains;
 
 namespace JsonApiDotNetCore.Queries.Parsing;
 
 /// <summary>
-/// Parses the JSON:API 'sort' and 'filter' query string parameter names, which contain a resource field chain that indicates the scope its query string
-/// parameter value applies to.
+/// Parser for the JSON:API 'sort' and 'filter' query string parameter names, which indicate the scope their query string parameter value applies to. The
+/// value consists of an optional relationship chain ending in a to-many relationship, surrounded by brackets.
 /// </summary>
 public interface IQueryStringParameterScopeParser
 {
@@ -20,11 +19,5 @@ public interface IQueryStringParameterScopeParser
     /// <param name="resourceType">
     /// The resource type used to lookup JSON:API fields that are referenced in <paramref name="source" />.
     /// </param>
-    /// <param name="pattern">
-    /// The pattern that the field chain in <paramref name="source" /> must match.
-    /// </param>
-    /// <param name="options">
-    /// The match options for <paramref name="pattern" />.
-    /// </param>
-    QueryStringParameterScopeExpression Parse(string source, ResourceType resourceType, FieldChainPattern pattern, FieldChainPatternMatchOptions options);
+    QueryStringParameterScopeExpression Parse(string source, ResourceType resourceType);
 }

src/JsonApiDotNetCore/Queries/Parsing/IncludeParser.cs

@@ -1,9 +1,5 @@
-using System.Collections.Immutable;
-using System.Collections.ObjectModel;
-using System.Text;
 using JetBrains.Annotations;
 using JsonApiDotNetCore.Configuration;
-using JsonApiDotNetCore.Errors;
 using JsonApiDotNetCore.Queries.Expressions;
 using JsonApiDotNetCore.Resources.Annotations;
 
@@ -29,190 +25,17 @@ public IncludeExpression Parse(string source, ResourceType resourceType)
 
         Tokenize(source);
 
-        IncludeExpression expression = ParseInclude(source, resourceType);
+        IncludeExpression expression = ParseInclude(resourceType);
 
         AssertTokenStackIsEmpty();
         ValidateMaximumIncludeDepth(expression, 0);
 
         return expression;
     }
 
-    protected virtual IncludeExpression ParseInclude(string source, ResourceType resourceType)
+    protected virtual IncludeExpression ParseInclude(ResourceType resourceType)
     {
-        ArgumentNullException.ThrowIfNull(source);
-        ArgumentNullException.ThrowIfNull(resourceType);
-
-        var treeRoot = IncludeTreeNode.CreateRoot(resourceType);
-        bool isAtStart = true;
-
-        while (TokenStack.Count > 0)
-        {
-            if (!isAtStart)
-            {
-                EatSingleCharacterToken(TokenKind.Comma);
-            }
-            else
-            {
-                isAtStart = false;
-            }
-
-            ParseRelationshipChain(source, treeRoot);
-        }
-
-        return treeRoot.ToExpression();
-    }
-
-    private void ParseRelationshipChain(string source, IncludeTreeNode treeRoot)
-    {
-        // A relationship name usually matches a single relationship, even when overridden in derived types.
-        // But in the following case, two relationships are matched on GET /shoppingBaskets?include=items:
-        //
-        // public abstract class ShoppingBasket : Identifiable<long>
-        // {
-        // }
-        //
-        // public sealed class SilverShoppingBasket : ShoppingBasket
-        // {
-        //     [HasMany]
-        //     public ISet<Article> Items { get; get; }
-        // }
-        //
-        // public sealed class PlatinumShoppingBasket : ShoppingBasket
-        // {
-        //     [HasMany]
-        //     public ISet<Product> Items { get; get; }
-        // }
-        //
-        // Now if the include chain has subsequent relationships, we need to scan both Items relationships for matches,
-        // which is why ParseRelationshipName returns a collection.
-        //
-        // The advantage of this unfolding is we don't require callers to upcast in relationship chains. The downside is
-        // that there's currently no way to include Products without Articles. We could add such optional upcast syntax
-        // in the future, if desired.
-
-        ReadOnlyCollection<IncludeTreeNode> children = ParseRelationshipName(source, [treeRoot]);
-
-        while (TokenStack.TryPeek(out Token? nextToken) && nextToken.Kind == TokenKind.Period)
-        {
-            EatSingleCharacterToken(TokenKind.Period);
-
-            children = ParseRelationshipName(source, children);
-        }
-    }
-
-    private ReadOnlyCollection<IncludeTreeNode> ParseRelationshipName(string source, IReadOnlyCollection<IncludeTreeNode> parents)
-    {
-        int position = GetNextTokenPositionOrEnd();
-
-        if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.Text)
-        {
-            return LookupRelationshipName(token.Value!, parents, source, position);
-        }
-
-        throw new QueryParseException("Relationship name expected.", position);
-    }
-
-    private static ReadOnlyCollection<IncludeTreeNode> LookupRelationshipName(string relationshipName, IReadOnlyCollection<IncludeTreeNode> parents,
-        string source, int position)
-    {
-        List<IncludeTreeNode> children = [];
-        HashSet<RelationshipAttribute> relationshipsFound = [];
-
-        foreach (IncludeTreeNode parent in parents)
-        {
-            // Depending on the left side of the include chain, we may match relationships anywhere in the resource type hierarchy.
-            // This is compensated for when rendering the response, which substitutes relationships on base types with the derived ones.
-            HashSet<RelationshipAttribute> relationships = GetRelationshipsInConcreteTypes(parent.Relationship.RightType, relationshipName);
-
-            if (relationships.Count > 0)
-            {
-                relationshipsFound.UnionWith(relationships);
-
-                RelationshipAttribute[] relationshipsToInclude = relationships.Where(relationship => !relationship.IsIncludeBlocked()).ToArray();
-                ReadOnlyCollection<IncludeTreeNode> affectedChildren = parent.EnsureChildren(relationshipsToInclude);
-                children.AddRange(affectedChildren);
-            }
-        }
-
-        AssertRelationshipsFound(relationshipsFound, relationshipName, parents, position);
-        AssertAtLeastOneCanBeIncluded(relationshipsFound, relationshipName, source, position);
-
-        return children.AsReadOnly();
-    }
-
-    private static HashSet<RelationshipAttribute> GetRelationshipsInConcreteTypes(ResourceType resourceType, string relationshipName)
-    {
-        HashSet<RelationshipAttribute> relationshipsToInclude = [];
-
-        foreach (RelationshipAttribute relationship in resourceType.GetRelationshipsInTypeOrDerived(relationshipName))
-        {
-            if (!relationship.LeftType.ClrType.IsAbstract)
-            {
-                relationshipsToInclude.Add(relationship);
-            }
-
-            IncludeRelationshipsFromConcreteDerivedTypes(relationship, relationshipsToInclude);
-        }
-
-        return relationshipsToInclude;
-    }
-
-    private static void IncludeRelationshipsFromConcreteDerivedTypes(RelationshipAttribute relationship, HashSet<RelationshipAttribute> relationshipsToInclude)
-    {
-        foreach (ResourceType derivedType in relationship.LeftType.GetAllConcreteDerivedTypes())
-        {
-            RelationshipAttribute relationshipInDerived = derivedType.GetRelationshipByPublicName(relationship.PublicName);
-            relationshipsToInclude.Add(relationshipInDerived);
-        }
-    }
-
-    private static void AssertRelationshipsFound(HashSet<RelationshipAttribute> relationshipsFound, string relationshipName,
-        IReadOnlyCollection<IncludeTreeNode> parents, int position)
-    {
-        if (relationshipsFound.Count > 0)
-        {
-            return;
-        }
-
-        ResourceType[] parentResourceTypes = parents.Select(parent => parent.Relationship.RightType).Distinct().ToArray();
-
-        bool hasDerivedTypes = parents.Any(parent => parent.Relationship.RightType.DirectlyDerivedTypes.Count > 0);
-
-        string message = GetErrorMessageForNoneFound(relationshipName, parentResourceTypes, hasDerivedTypes);
-        throw new QueryParseException(message, position);
-    }
-
-    private static string GetErrorMessageForNoneFound(string relationshipName, ResourceType[] parentResourceTypes, bool hasDerivedTypes)
-    {
-        var builder = new StringBuilder($"Relationship '{relationshipName}'");
-
-        if (parentResourceTypes.Length == 1)
-        {
-            builder.Append($" does not exist on resource type '{parentResourceTypes.First().PublicName}'");
-        }
-        else
-        {
-            string typeNames = string.Join(", ", parentResourceTypes.Select(type => $"'{type.PublicName}'"));
-            builder.Append($" does not exist on any of the resource types {typeNames}");
-        }
-
-        builder.Append(hasDerivedTypes ? " or any of its derived types." : ".");
-
-        return builder.ToString();
-    }
-
-    private static void AssertAtLeastOneCanBeIncluded(HashSet<RelationshipAttribute> relationshipsFound, string relationshipName, string source, int position)
-    {
-        if (relationshipsFound.All(relationship => relationship.IsIncludeBlocked()))
-        {
-            ResourceType resourceType = relationshipsFound.First().LeftType;
-            string message = $"Including the relationship '{relationshipName}' on '{resourceType}' is not allowed.";
-
-            var exception = new QueryParseException(message, position);
-            string specificMessage = exception.GetMessageWithPosition(source);
-
-            throw new InvalidQueryStringParameterException("include", "The specified include is invalid.", specificMessage);
-        }
+        return ParseCommaSeparatedSequenceOfRelationshipChains(resourceType);
     }
 
     private void ValidateMaximumIncludeDepth(IncludeExpression include, int position)
@@ -247,71 +70,4 @@ private static void ThrowIfMaximumDepthExceeded(IncludeElementExpression include
 
         parentChain.Pop();
     }
-
-    private sealed class IncludeTreeNode
-    {
-        private readonly Dictionary<RelationshipAttribute, IncludeTreeNode> _children = [];
-
-        public RelationshipAttribute Relationship { get; }
-
-        private IncludeTreeNode(RelationshipAttribute relationship)
-        {
-            Relationship = relationship;
-        }
-
-        public static IncludeTreeNode CreateRoot(ResourceType resourceType)
-        {
-            var relationship = new HiddenRootRelationshipAttribute(resourceType);
-            return new IncludeTreeNode(relationship);
-        }
-
-        public ReadOnlyCollection<IncludeTreeNode> EnsureChildren(RelationshipAttribute[] relationships)
-        {
-            foreach (RelationshipAttribute relationship in relationships)
-            {
-                if (!_children.ContainsKey(relationship))
-                {
-                    var newChild = new IncludeTreeNode(relationship);
-                    _children.Add(relationship, newChild);
-                }
-            }
-
-            return _children.Where(pair => relationships.Contains(pair.Key)).Select(pair => pair.Value).ToArray().AsReadOnly();
-        }
-
-        public IncludeExpression ToExpression()
-        {
-            IncludeElementExpression element = ToElementExpression();
-
-            if (element.Relationship is HiddenRootRelationshipAttribute)
-            {
-                return element.Children.Count > 0 ? new IncludeExpression(element.Children) : IncludeExpression.Empty;
-            }
-
-            return new IncludeExpression(ImmutableHashSet.Create(element));
-        }
-
-        private IncludeElementExpression ToElementExpression()
-        {
-            IImmutableSet<IncludeElementExpression> elementChildren = _children.Values.Select(child => child.ToElementExpression()).ToImmutableHashSet();
-            return new IncludeElementExpression(Relationship, elementChildren);
-        }
-
-        public override string ToString()
-        {
-            IncludeExpression include = ToExpression();
-            return include.ToFullString();
-        }
-
-        private sealed class HiddenRootRelationshipAttribute : RelationshipAttribute
-        {
-            public HiddenRootRelationshipAttribute(ResourceType rightType)
-            {
-                ArgumentNullException.ThrowIfNull(rightType);
-
-                RightType = rightType;
-                PublicName = "<<root>>";
-            }
-        }
-    }
 }

src/JsonApiDotNetCore/Queries/Parsing/PaginationParser.cs

@@ -2,7 +2,6 @@
 using JetBrains.Annotations;
 using JsonApiDotNetCore.Configuration;
 using JsonApiDotNetCore.Queries.Expressions;
-using JsonApiDotNetCore.QueryStrings.FieldChains;
 
 namespace JsonApiDotNetCore.Queries.Parsing;
 
@@ -57,8 +56,7 @@ protected virtual PaginationElementQueryStringValueExpression ParsePaginationEle
             return new PaginationElementQueryStringValueExpression(null, number.Value, position);
         }
 
-        ResourceFieldChainExpression scope = ParseFieldChain(BuiltInPatterns.RelationshipChainEndingInToMany, FieldChainPatternMatchOptions.None, resourceType,
-            "Number or relationship name expected.");
+        IncludeExpression scope = ParseRelationshipChainEndingInToMany(resourceType, "Number or relationship name expected.");
 
         EatSingleCharacterToken(TokenKind.Colon);