Fix empty fields (#1009)

Bart Koelman · web-flow · commit f145564e0233 · 2021-06-11T13:49:25.000+02:00
* Allows incoming empty fieldset (json:api spec compliance). The code adds a default interface property to not break existing implementations. * Workaround for bug dotnet/roslyn#13624
diff --git a/src/JsonApiDotNetCore/Middleware/FixedQueryFeature.cs b/src/JsonApiDotNetCore/Middleware/FixedQueryFeature.cs
@@ -0,0 +1,81 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Http.Features;
+using Microsoft.Extensions.Primitives;
+
+namespace JsonApiDotNetCore.Middleware
+{
+    /// <summary>
+    /// Replacement implementation for the ASP.NET built-in <see cref="QueryFeature" />, to workaround bug https://github.com/dotnet/aspnetcore/issues/33394.
+    /// This is identical to the built-in version, except it calls <see cref="FixedQueryHelpers.ParseNullableQuery" />.
+    /// </summary>
+    internal sealed class FixedQueryFeature : IQueryFeature
+    {
+        // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624
+        private static readonly Func<IFeatureCollection, IHttpRequestFeature> NullRequestFeature = _ => null;
+
+        private FeatureReferences<IHttpRequestFeature> _features;
+
+        private string _original;
+        private IQueryCollection _parsedValues;
+
+        private IHttpRequestFeature HttpRequestFeature => _features.Fetch(ref _features.Cache, NullRequestFeature);
+
+        /// <inheritdoc />
+        public IQueryCollection Query
+        {
+            get
+            {
+                if (_features.Collection == null)
+                {
+                    return _parsedValues ??= QueryCollection.Empty;
+                }
+
+                string current = HttpRequestFeature.QueryString;
+
+                if (_parsedValues == null || !string.Equals(_original, current, StringComparison.Ordinal))
+                {
+                    _original = current;
+
+                    Dictionary<string, StringValues> result = FixedQueryHelpers.ParseNullableQuery(current);
+
+                    _parsedValues = result == null ? QueryCollection.Empty : new QueryCollection(result);
+                }
+
+                return _parsedValues;
+            }
+            set
+            {
+                _parsedValues = value;
+
+                if (_features.Collection != null)
+                {
+                    if (value == null)
+                    {
+                        _original = string.Empty;
+                        HttpRequestFeature.QueryString = string.Empty;
+                    }
+                    else
+                    {
+                        _original = QueryString.Create(_parsedValues).ToString();
+                        HttpRequestFeature.QueryString = _original;
+                    }
+                }
+            }
+        }
+
+        /// <summary>
+        /// Initializes a new instance of <see cref="QueryFeature" />.
+        /// </summary>
+        /// <param name="features">
+        /// The <see cref="IFeatureCollection" /> to initialize.
+        /// </param>
+        public FixedQueryFeature(IFeatureCollection features)
+        {
+            ArgumentGuard.NotNull(features, nameof(features));
+
+            _features.Initalize(features);
+        }
+    }
+}
diff --git a/src/JsonApiDotNetCore/Middleware/FixedQueryHelpers.cs b/src/JsonApiDotNetCore/Middleware/FixedQueryHelpers.cs
@@ -0,0 +1,102 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.AspNetCore.WebUtilities;
+using Microsoft.Extensions.Primitives;
+
+#pragma warning disable AV1008 // Class should not be static
+#pragma warning disable AV1708 // Type name contains term that should be avoided
+#pragma warning disable AV1130 // Return type in method signature should be a collection interface instead of a concrete type
+#pragma warning disable AV1532 // Loop statement contains nested loop
+
+namespace JsonApiDotNetCore.Middleware
+{
+    /// <summary>
+    /// Replacement implementation for the ASP.NET built-in <see cref="QueryHelpers" />, to workaround bug https://github.com/dotnet/aspnetcore/issues/33394.
+    /// This is identical to the built-in version, except it properly un-escapes query string keys without a value.
+    /// </summary>
+    internal static class FixedQueryHelpers
+    {
+        /// <summary>
+        /// Parse a query string into its component key and value parts.
+        /// </summary>
+        /// <param name="queryString">
+        /// The raw query string value, with or without the leading '?'.
+        /// </param>
+        /// <returns>
+        /// A collection of parsed keys and values, null if there are no entries.
+        /// </returns>
+        public static Dictionary<string, StringValues> ParseNullableQuery(string queryString)
+        {
+            var accumulator = new KeyValueAccumulator();
+
+            if (string.IsNullOrEmpty(queryString) || queryString == "?")
+            {
+                return null;
+            }
+
+            int scanIndex = 0;
+
+            if (queryString[0] == '?')
+            {
+                scanIndex = 1;
+            }
+
+            int textLength = queryString.Length;
+            int equalIndex = queryString.IndexOf('=');
+
+            if (equalIndex == -1)
+            {
+                equalIndex = textLength;
+            }
+
+            while (scanIndex < textLength)
+            {
+                int delimiterIndex = queryString.IndexOf('&', scanIndex);
+
+                if (delimiterIndex == -1)
+                {
+                    delimiterIndex = textLength;
+                }
+
+                if (equalIndex < delimiterIndex)
+                {
+                    while (scanIndex != equalIndex && char.IsWhiteSpace(queryString[scanIndex]))
+                    {
+                        ++scanIndex;
+                    }
+
+                    string name = queryString.Substring(scanIndex, equalIndex - scanIndex);
+                    string value = queryString.Substring(equalIndex + 1, delimiterIndex - equalIndex - 1);
+                    accumulator.Append(Uri.UnescapeDataString(name.Replace('+', ' ')), Uri.UnescapeDataString(value.Replace('+', ' ')));
+                    equalIndex = queryString.IndexOf('=', delimiterIndex);
+
+                    if (equalIndex == -1)
+                    {
+                        equalIndex = textLength;
+                    }
+                }
+                else
+                {
+                    if (delimiterIndex > scanIndex)
+                    {
+                        // original code:
+                        // accumulator.Append(queryString.Substring(scanIndex, delimiterIndex - scanIndex), string.Empty);
+
+                        // replacement:
+                        string name = queryString.Substring(scanIndex, delimiterIndex - scanIndex);
+                        accumulator.Append(Uri.UnescapeDataString(name.Replace('+', ' ')), string.Empty);
+                    }
+                }
+
+                scanIndex = delimiterIndex + 1;
+            }
+
+            if (!accumulator.HasValues)
+            {
+                return null;
+            }
+
+            return accumulator.GetResults();
+        }
+    }
+}
diff --git a/src/JsonApiDotNetCore/Middleware/JsonApiMiddleware.cs b/src/JsonApiDotNetCore/Middleware/JsonApiMiddleware.cs
@@ -12,6 +12,7 @@
 using JsonApiDotNetCore.Serialization;
 using JsonApiDotNetCore.Serialization.Objects;
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.Controllers;
 using Microsoft.AspNetCore.Routing;
@@ -78,6 +79,9 @@ public async Task InvokeAsync(HttpContext httpContext, IControllerResourceMappin
                 httpContext.RegisterJsonApiRequest();
             }
 
+            // Workaround for bug https://github.com/dotnet/aspnetcore/issues/33394
+            httpContext.Features.Set<IQueryFeature>(new FixedQueryFeature(httpContext.Features));
+
             await _next(httpContext);
         }
 
diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/SparseFieldSetParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/SparseFieldSetParser.cs
@@ -40,20 +40,19 @@ protected SparseFieldSetExpression ParseSparseFieldSet()
         {
             var fields = new Dictionary<string, ResourceFieldAttribute>();
 
-            ResourceFieldChainExpression nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");
-            ResourceFieldAttribute nextField = nextChain.Fields.Single();
-            fields[nextField.PublicName] = nextField;
-
             while (TokenStack.Any())
             {
-                EatSingleCharacterToken(TokenKind.Comma);
+                if (fields.Count > 0)
+                {
+                    EatSingleCharacterToken(TokenKind.Comma);
+                }
 
-                nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");
-                nextField = nextChain.Fields.Single();
+                ResourceFieldChainExpression nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");
+                ResourceFieldAttribute nextField = nextChain.Fields.Single();
                 fields[nextField.PublicName] = nextField;
             }
 
-            return new SparseFieldSetExpression(fields.Values);
+            return fields.Any() ? new SparseFieldSetExpression(fields.Values) : null;
         }
 
         protected override IReadOnlyCollection<ResourceFieldAttribute> OnResolveFieldChain(string path, FieldChainRequirements chainRequirements)
diff --git a/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs b/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs
@@ -8,6 +8,11 @@ namespace JsonApiDotNetCore.QueryStrings
     /// </summary>
     public interface IQueryStringParameterReader
     {
+        /// <summary>
+        /// Indicates whether this reader supports empty query string parameter values. Defaults to <c>false</c>.
+        /// </summary>
+        bool AllowEmptyValue => false;
+
         /// <summary>
         /// Indicates whether usage of this query string parameter is blocked using <see cref="DisableQueryStringAttribute" /> on a controller.
         /// </summary>
diff --git a/src/JsonApiDotNetCore/QueryStrings/Internal/QueryStringReader.cs b/src/JsonApiDotNetCore/QueryStrings/Internal/QueryStringReader.cs
@@ -39,18 +39,18 @@ public virtual void ReadAll(DisableQueryStringAttribute disableQueryStringAttrib
 
             foreach ((string parameterName, StringValues parameterValue) in _queryStringAccessor.Query)
             {
-                if (string.IsNullOrEmpty(parameterValue))
-                {
-                    throw new InvalidQueryStringParameterException(parameterName, "Missing query string parameter value.",
-                        $"Missing value for '{parameterName}' query string parameter.");
-                }
-
                 IQueryStringParameterReader reader = _parameterReaders.FirstOrDefault(nextReader => nextReader.CanRead(parameterName));
 
                 if (reader != null)
                 {
                     _logger.LogDebug($"Query string parameter '{parameterName}' with value '{parameterValue}' was accepted by {reader.GetType().Name}.");
 
+                    if (!reader.AllowEmptyValue && string.IsNullOrEmpty(parameterValue))
+                    {
+                        throw new InvalidQueryStringParameterException(parameterName, "Missing query string parameter value.",
+                            $"Missing value for '{parameterName}' query string parameter.");
+                    }
+
                     if (!reader.IsEnabled(disableQueryStringAttributeNotNull))
                     {
                         throw new InvalidQueryStringParameterException(parameterName,
diff --git a/src/JsonApiDotNetCore/QueryStrings/Internal/SparseFieldSetQueryStringParameterReader.cs b/src/JsonApiDotNetCore/QueryStrings/Internal/SparseFieldSetQueryStringParameterReader.cs
@@ -9,6 +9,7 @@
 using JsonApiDotNetCore.Queries;
 using JsonApiDotNetCore.Queries.Expressions;
 using JsonApiDotNetCore.Queries.Internal.Parsing;
+using JsonApiDotNetCore.Resources;
 using JsonApiDotNetCore.Resources.Annotations;
 using Microsoft.Extensions.Primitives;
 
@@ -22,6 +23,9 @@ public class SparseFieldSetQueryStringParameterReader : QueryStringParameterRead
         private readonly Dictionary<ResourceContext, SparseFieldSetExpression> _sparseFieldTable = new Dictionary<ResourceContext, SparseFieldSetExpression>();
         private string _lastParameterName;
 
+        /// <inheritdoc />
+        bool IQueryStringParameterReader.AllowEmptyValue => true;
+
         public SparseFieldSetQueryStringParameterReader(IJsonApiRequest request, IResourceContextProvider resourceContextProvider)
             : base(request, resourceContextProvider)
         {
@@ -79,7 +83,16 @@ private ResourceContext GetSparseFieldType(string parameterName)
 
         private SparseFieldSetExpression GetSparseFieldSet(string parameterValue, ResourceContext resourceContext)
         {
-            return _sparseFieldSetParser.Parse(parameterValue, resourceContext);
+            SparseFieldSetExpression sparseFieldSet = _sparseFieldSetParser.Parse(parameterValue, resourceContext);
+
+            if (sparseFieldSet == null)
+            {
+                // We add ID on an incoming empty fieldset, so that callers can distinguish between no fieldset and an empty one.
+                AttrAttribute idAttribute = resourceContext.Attributes.Single(attribute => attribute.Property.Name == nameof(Identifiable.Id));
+                return new SparseFieldSetExpression(ArrayFactory.Create(idAttribute));
+            }
+
+            return sparseFieldSet;
         }
 
         /// <inheritdoc />
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/QueryStringTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/QueryStringTests.cs
@@ -69,8 +69,8 @@ public async Task Can_use_unknown_query_string_parameter()
         [InlineData("include")]
         [InlineData("filter")]
         [InlineData("sort")]
-        [InlineData("page")]
-        [InlineData("fields")]
+        [InlineData("page[size]")]
+        [InlineData("page[number]")]
         [InlineData("defaults")]
         [InlineData("nulls")]
         public async Task Cannot_use_empty_query_string_parameter_value(string parameterName)
diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/SparseFieldSets/SparseFieldSetTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/SparseFieldSets/SparseFieldSetTests.cs
@@ -579,6 +579,40 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
             postCaptured.Url.Should().BeNull();
         }
 
+        [Fact]
+        public async Task Can_select_empty_fieldset()
+        {
+            // Arrange
+            var store = _testContext.Factory.Services.GetRequiredService<ResourceCaptureStore>();
+            store.Clear();
+
+            BlogPost post = _fakers.BlogPost.Generate();
+
+            await _testContext.RunOnDatabaseAsync(async dbContext =>
+            {
+                await dbContext.ClearTableAsync<BlogPost>();
+                dbContext.Posts.Add(post);
+                await dbContext.SaveChangesAsync();
+            });
+
+            const string route = "/blogPosts?fields[blogPosts]=";
+
+            // Act
+            (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync<Document>(route);
+
+            // Assert
+            httpResponse.Should().HaveStatusCode(HttpStatusCode.OK);
+
+            responseDocument.ManyData.Should().HaveCount(1);
+            responseDocument.ManyData[0].Id.Should().Be(post.StringId);
+            responseDocument.ManyData[0].Attributes.Should().BeNull();
+            responseDocument.ManyData[0].Relationships.Should().BeNull();
+
+            var postCaptured = (BlogPost)store.Resources.Should().ContainSingle(resource => resource is BlogPost).And.Subject.Single();
+            postCaptured.Id.Should().Be(post.Id);
+            postCaptured.Url.Should().BeNull();
+        }
+
         [Fact]
         public async Task Cannot_select_on_unknown_resource_type()
         {
diff --git a/test/JsonApiDotNetCoreExampleTests/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs b/test/JsonApiDotNetCoreExampleTests/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs
@@ -60,7 +60,6 @@ public void Reader_Is_Enabled(StandardQueryStringParameters parametersDisabled,
         [InlineData("fields[ ]", "", "Unexpected whitespace.")]
         [InlineData("fields[owner]", "", "Resource type 'owner' does not exist.")]
         [InlineData("fields[owner.posts]", "id", "Resource type 'owner.posts' does not exist.")]
-        [InlineData("fields[blogPosts]", "", "Field name expected.")]
         [InlineData("fields[blogPosts]", " ", "Unexpected whitespace.")]
         [InlineData("fields[blogPosts]", "some", "Field 'some' does not exist on resource 'blogPosts'.")]
         [InlineData("fields[blogPosts]", "id,owner.name", "Field 'owner.name' does not exist on resource 'blogPosts'.")]
@@ -87,6 +86,7 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin
         [InlineData("fields[blogPosts]", "caption,url,author", "blogPosts(caption,url,author)")]
         [InlineData("fields[blogPosts]", "author,comments,labels", "blogPosts(author,comments,labels)")]
         [InlineData("fields[blogs]", "id", "blogs(id)")]
+        [InlineData("fields[blogs]", "", "blogs(id)")]
         public void Reader_Read_Succeeds(string parameterName, string parameterValue, string valueExpected)
         {
             // Act

Original file line number	Diff line number	Diff line change
`@@ -40,20 +40,19 @@ protected SparseFieldSetExpression ParseSparseFieldSet()`
`40`	`40`	`{`
`41`	`41`	`var fields = new Dictionary<string, ResourceFieldAttribute>();`
`42`	`42`
`43`		`- ResourceFieldChainExpression nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");`
`44`		`- ResourceFieldAttribute nextField = nextChain.Fields.Single();`
`45`		`- fields[nextField.PublicName] = nextField;`
`46`		`-`
`47`	`43`	`while (TokenStack.Any())`
`48`	`44`	`{`
`49`		`- EatSingleCharacterToken(TokenKind.Comma);`
	`45`	`+ if (fields.Count > 0)`
	`46`	`+ {`
	`47`	`+ EatSingleCharacterToken(TokenKind.Comma);`
	`48`	`+ }`
`50`	`49`
`51`		`- nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");`
`52`		`- nextField = nextChain.Fields.Single();`
	`50`	`+ ResourceFieldChainExpression nextChain = ParseFieldChain(FieldChainRequirements.EndsInAttribute, "Field name expected.");`
	`51`	`+ ResourceFieldAttribute nextField = nextChain.Fields.Single();`
`53`	`52`	`fields[nextField.PublicName] = nextField;`
`54`	`53`	`}`
`55`	`54`
`56`		`- return new SparseFieldSetExpression(fields.Values);`
	`55`	`+ return fields.Any() ? new SparseFieldSetExpression(fields.Values) : null;`
`57`	`56`	`}`
`58`	`57`
`59`	`58`	`protected override IReadOnlyCollection<ResourceFieldAttribute> OnResolveFieldChain(string path, FieldChainRequirements chainRequirements)`