Fixed: do not allow comparison of count() with null; do not treat null as a field name

Author: bkoelman

src/JsonApiDotNetCore/Queries/Internal/Parsing/FilterParser.cs

@@ -135,40 +135,34 @@ protected ComparisonExpression ParseComparison(string operatorName)
         EatText(operatorName);
         EatSingleCharacterToken(TokenKind.OpenParen);
 
-        // Allow equality comparison of a HasOne relationship with null.
+        // Allow equality comparison of a to-one relationship with null.
         FieldChainRequirements leftChainRequirements = comparisonOperator == ComparisonOperator.Equals
             ? FieldChainRequirements.EndsInAttribute | FieldChainRequirements.EndsInToOne
             : FieldChainRequirements.EndsInAttribute;
 
         QueryExpression leftTerm = ParseCountOrField(leftChainRequirements);
-        Converter<string, object> rightConstantValueConverter;
+
+        EatSingleCharacterToken(TokenKind.Comma);
+
+        QueryExpression rightTerm;
 
         if (leftTerm is CountExpression)
         {
-            rightConstantValueConverter = GetConstantValueConverterForCount();
+            Converter<string, object> rightConstantValueConverter = GetConstantValueConverterForCount();
+            rightTerm = ParseCountOrConstantOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter);
         }
         else if (leftTerm is ResourceFieldChainExpression fieldChain && fieldChain.Fields[^1] is AttrAttribute attribute)
         {
-            rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute);
+            Converter<string, object> rightConstantValueConverter = GetConstantValueConverterForAttribute(attribute);
+            rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter);
         }
         else
         {
-            // This temporary value never survives; it gets discarded during the second pass below.
-            rightConstantValueConverter = _ => 0;
+            rightTerm = ParseNull();
         }
 
-        EatSingleCharacterToken(TokenKind.Comma);
-
-        QueryExpression rightTerm = ParseCountOrConstantOrNullOrField(FieldChainRequirements.EndsInAttribute, rightConstantValueConverter);
-
         EatSingleCharacterToken(TokenKind.CloseParen);
 
-        if (leftTerm is ResourceFieldChainExpression leftChain && leftChain.Fields[^1] is RelationshipAttribute && rightTerm is not NullConstantExpression)
-        {
-            // Run another pass over left chain to produce an error.
-            OnResolveFieldChain(leftChain.ToString(), FieldChainRequirements.EndsInAttribute);
-        }
-
         return new ComparisonExpression(comparisonOperator, leftTerm, rightTerm);
     }
 
@@ -201,8 +195,9 @@ protected AnyExpression ParseAny()
         EatText(Keywords.Any);
         EatSingleCharacterToken(TokenKind.OpenParen);
 
-        ResourceFieldChainExpression targetAttribute = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null);
-        Converter<string, object> constantValueConverter = GetConstantValueConverterForAttribute((AttrAttribute)targetAttribute.Fields[^1]);
+        ResourceFieldChainExpression targetAttributeChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, null);
+        var targetAttribute = (AttrAttribute)targetAttributeChain.Fields[^1];
+        Converter<string, object> constantValueConverter = GetConstantValueConverterForAttribute(targetAttribute);
 
         EatSingleCharacterToken(TokenKind.Comma);
 
@@ -223,7 +218,7 @@ protected AnyExpression ParseAny()
 
         IImmutableSet<LiteralConstantExpression> constantSet = constantsBuilder.ToImmutable();
 
-        return new AnyExpression(targetAttribute, constantSet);
+        return new AnyExpression(targetAttributeChain, constantSet);
     }
 
     protected HasExpression ParseHas()
@@ -349,6 +344,25 @@ protected QueryExpression ParseCountOrField(FieldChainRequirements chainRequirem
         return ParseFieldChain(chainRequirements, "Count function or field name expected.");
     }
 
+    protected QueryExpression ParseCountOrConstantOrField(FieldChainRequirements chainRequirements, Converter<string, object> constantValueConverter)
+    {
+        CountExpression? count = TryParseCount();
+
+        if (count != null)
+        {
+            return count;
+        }
+
+        LiteralConstantExpression? constant = TryParseConstant(constantValueConverter);
+
+        if (constant != null)
+        {
+            return constant;
+        }
+
+        return ParseFieldChain(chainRequirements, "Count function, value between quotes or field name expected.");
+    }
+
     protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequirements chainRequirements, Converter<string, object> constantValueConverter)
     {
         CountExpression? count = TryParseCount();
@@ -368,6 +382,19 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen
         return ParseFieldChain(chainRequirements, "Count function, value between quotes, null or field name expected.");
     }
 
+    protected LiteralConstantExpression? TryParseConstant(Converter<string, object> constantValueConverter)
+    {
+        if (TokenStack.TryPeek(out Token? nextToken) && nextToken.Kind == TokenKind.QuotedText)
+        {
+            TokenStack.Pop();
+
+            object constantValue = constantValueConverter(nextToken.Value!);
+            return new LiteralConstantExpression(constantValue, nextToken.Value!);
+        }
+
+        return null;
+    }
+
     protected IdentifierExpression? TryParseConstantOrNull(Converter<string, object> constantValueConverter)
     {
         if (TokenStack.TryPeek(out Token? nextToken))
@@ -392,13 +419,24 @@ protected QueryExpression ParseCountOrConstantOrNullOrField(FieldChainRequiremen
 
     protected LiteralConstantExpression ParseConstant(Converter<string, object> constantValueConverter)
     {
-        if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.QuotedText)
+        LiteralConstantExpression? constant = TryParseConstant(constantValueConverter);
+
+        if (constant == null)
+        {
+            throw new QueryParseException("Value between quotes expected.");
+        }
+
+        return constant;
+    }
+
+    protected NullConstantExpression ParseNull()
+    {
+        if (TokenStack.TryPop(out Token? token) && token is { Kind: TokenKind.Text, Value: Keywords.Null })
         {
-            object constantValue = constantValueConverter(token.Value!);
-            return new LiteralConstantExpression(constantValue, token.Value!);
+            return NullConstantExpression.Instance;
         }
 
-        throw new QueryParseException("Value between quotes expected.");
+        throw new QueryParseException("null expected.");
     }
 
     private Converter<string, object> GetConstantValueConverterForCount()

src/JsonApiDotNetCore/Queries/Internal/Parsing/QueryExpressionParser.cs

@@ -49,7 +49,7 @@ private void EatFieldChain(StringBuilder pathBuilder, string? alternativeErrorMe
     {
         while (true)
         {
-            if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.Text)
+            if (TokenStack.TryPop(out Token? token) && token.Kind == TokenKind.Text && token.Value != Keywords.Null)
             {
                 pathBuilder.Append(token.Value);
 

test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/FilterParseTests.cs

@@ -63,7 +63,7 @@ public void Reader_Is_Enabled(JsonApiQueryStringParameters parametersDisabled, b
         "Relationship 'author' in 'posts.author' must be a to-many relationship on resource type 'blogPosts'.")]
     [InlineData("filter[posts.comments.author]", "equals(firstName,'some')",
         "Relationship 'author' in 'posts.comments.author' must be a to-many relationship on resource type 'comments'.")]
-    [InlineData("filter[posts]", "equals(author,'some')", "Attribute 'author' does not exist on resource type 'blogPosts'.")]
+    [InlineData("filter[posts]", "equals(author,'some')", "null expected.")]
     [InlineData("filter[posts]", "lessThan(author,null)", "Attribute 'author' does not exist on resource type 'blogPosts'.")]
     [InlineData("filter", " ", "Unexpected whitespace.")]
     [InlineData("filter", "contains(owner.displayName ,)", "Unexpected whitespace.")]
@@ -73,20 +73,21 @@ public void Reader_Is_Enabled(JsonApiQueryStringParameters parametersDisabled, b
     [InlineData("filter", "equals'", "Unexpected ' outside text.")]
     [InlineData("filter", "equals(", "Count function or field name expected.")]
     [InlineData("filter", "equals('1'", "Count function or field name expected.")]
-    [InlineData("filter", "equals(count(posts),", "Count function, value between quotes, null or field name expected.")]
+    [InlineData("filter", "equals(count(posts),", "Count function, value between quotes or field name expected.")]
+    [InlineData("filter", "equals(count(posts),null)", "Count function, value between quotes or field name expected.")]
     [InlineData("filter", "equals(owner..displayName,'')", "Count function or field name expected.")]
     [InlineData("filter", "equals(owner.displayName.,'')", "Count function or field name expected.")]
     [InlineData("filter", "equals(title,')", "' expected.")]
     [InlineData("filter", "equals(title,null", ") expected.")]
-    [InlineData("filter", "equals(null", "Field 'null' does not exist on resource type 'blogs'.")]
+    [InlineData("filter", "equals(null", "Count function or field name expected.")]
     [InlineData("filter", "equals(title,(", "Count function, value between quotes, null or field name expected.")]
     [InlineData("filter", "equals(has(posts),'true')", "Field 'has' does not exist on resource type 'blogs'.")]
     [InlineData("filter", "has(posts,", "Filter function expected.")]
     [InlineData("filter", "contains)", "( expected.")]
     [InlineData("filter", "contains(title,'a','b')", ") expected.")]
     [InlineData("filter", "contains(title,null)", "Value between quotes expected.")]
     [InlineData("filter[posts]", "contains(author,null)", "Attribute 'author' does not exist on resource type 'blogPosts'.")]
-    [InlineData("filter", "any(null,'a','b')", "Attribute 'null' does not exist on resource type 'blogs'.")]
+    [InlineData("filter", "any(null,'a','b')", "Field name expected.")]
     [InlineData("filter", "any('a','b','c')", "Field name expected.")]
     [InlineData("filter", "any(title,'b','c',)", "Value between quotes expected.")]
     [InlineData("filter", "any(title)", ", expected.")]
@@ -137,6 +138,7 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin
     [InlineData("filter[owner.posts]", "equals(caption,'some')", "owner.posts", "equals(caption,'some')")]
     [InlineData("filter[posts.comments]", "equals(createdAt,'2000-01-01')", "posts.comments", "equals(createdAt,'2000-01-01')")]
     [InlineData("filter", "equals(count(posts),'1')", null, "equals(count(posts),'1')")]
+    [InlineData("filter", "equals(count(posts),count(owner.posts))", null, "equals(count(posts),count(owner.posts))")]
     [InlineData("filter[posts]", "equals(caption,null)", "posts", "equals(caption,null)")]
     [InlineData("filter[posts]", "equals(author,null)", "posts", "equals(author,null)")]
     [InlineData("filter[posts]", "equals(author.userName,author.displayName)", "posts", "equals(author.userName,author.displayName)")]

test/JsonApiDotNetCoreTests/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs

@@ -32,12 +32,12 @@ public LegacyFilterParseTests()
     [InlineData("filter[", "some", "Expected field name between brackets in filter parameter name.")]
     [InlineData("filter[]", "some", "Expected field name between brackets in filter parameter name.")]
     [InlineData("filter[some]", "other", "Field 'some' does not exist on resource type 'blogPosts'.")]
-    [InlineData("filter[author]", "some", "Attribute 'author' does not exist on resource type 'blogPosts'.")]
+    [InlineData("filter[author]", "some", "null expected.")]
     [InlineData("filter[author.posts]", "some",
         "Field 'posts' in 'author.posts' must be an attribute or a to-one relationship on resource type 'webAccounts'.")]
     [InlineData("filter[unknown.id]", "some", "Relationship 'unknown' in 'unknown.id' does not exist on resource type 'blogPosts'.")]
     [InlineData("filter", "expr:equals(some,'other')", "Field 'some' does not exist on resource type 'blogPosts'.")]
-    [InlineData("filter", "expr:equals(author,'Joe')", "Attribute 'author' does not exist on resource type 'blogPosts'.")]
+    [InlineData("filter", "expr:equals(author,'Joe')", "null expected.")]
     [InlineData("filter", "expr:has(author)", "Relationship 'author' must be a to-many relationship on resource type 'blogPosts'.")]
     [InlineData("filter", "expr:equals(count(author),'1')", "Relationship 'author' must be a to-many relationship on resource type 'blogPosts'.")]
     public void Reader_Read_Fails(string parameterName, string parameterValue, string errorMessage)