From f036db7bd8031c1e844a59ca5290ec30669740c1 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Tue, 8 Jun 2021 18:57:28 +0200 Subject: [PATCH 1/2] Allows incoming empty fieldset (json:api spec compliance). The code adds a default interface property to not break existing implementations. --- .../Internal/Parsing/SparseFieldSetParser.cs | 15 ++++---- .../IQueryStringParameterReader.cs | 5 +++ .../Internal/QueryStringReader.cs | 12 +++---- ...parseFieldSetQueryStringParameterReader.cs | 15 +++++++- .../QueryStrings/QueryStringTests.cs | 4 +-- .../SparseFieldSets/SparseFieldSetTests.cs | 34 +++++++++++++++++++ .../SparseFieldSetParseTests.cs | 2 +- 7 files changed, 69 insertions(+), 18 deletions(-) diff --git a/src/JsonApiDotNetCore/Queries/Internal/Parsing/SparseFieldSetParser.cs b/src/JsonApiDotNetCore/Queries/Internal/Parsing/SparseFieldSetParser.cs index 52fbe6610b..7ce7168a0d 100644 --- 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(); - 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 OnResolveFieldChain(string path, FieldChainRequirements chainRequirements) diff --git a/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs b/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs index a7688bf8f7..024ee564f2 100644 --- a/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs +++ b/src/JsonApiDotNetCore/QueryStrings/IQueryStringParameterReader.cs @@ -8,6 +8,11 @@ namespace JsonApiDotNetCore.QueryStrings /// public interface IQueryStringParameterReader { + /// + /// Indicates whether this reader supports empty query string parameter values. Defaults to false. + /// + bool AllowEmptyValue => false; + /// /// Indicates whether usage of this query string parameter is blocked using on a controller. /// diff --git a/src/JsonApiDotNetCore/QueryStrings/Internal/QueryStringReader.cs b/src/JsonApiDotNetCore/QueryStrings/Internal/QueryStringReader.cs index 19673f7fb3..fe4064a9c2 100644 --- 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 index 1ab65dc332..f9e804797c 100644 --- 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 _sparseFieldTable = new Dictionary(); private string _lastParameterName; + /// + 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; } /// diff --git a/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/QueryStringTests.cs b/test/JsonApiDotNetCoreExampleTests/IntegrationTests/QueryStrings/QueryStringTests.cs index d7a346d6a1..2ff57e16f4 100644 --- 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 index 8518a9237b..6fc881e6f1 100644 --- 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(); + store.Clear(); + + BlogPost post = _fakers.BlogPost.Generate(); + + await _testContext.RunOnDatabaseAsync(async dbContext => + { + await dbContext.ClearTableAsync(); + dbContext.Posts.Add(post); + await dbContext.SaveChangesAsync(); + }); + + const string route = "/blogPosts?fields[blogPosts]="; + + // Act + (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecuteGetAsync(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 index 70cd830ce0..42e63a5a84 100644 --- 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 From afb02c0039e33a18f9683469b90ab1838de4c721 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Wed, 9 Jun 2021 10:18:46 +0200 Subject: [PATCH 2/2] Workaround for bug https://github.com/dotnet/roslyn/issues/13624 --- .../Middleware/FixedQueryFeature.cs | 81 ++++++++++++++ .../Middleware/FixedQueryHelpers.cs | 102 ++++++++++++++++++ .../Middleware/JsonApiMiddleware.cs | 4 + 3 files changed, 187 insertions(+) create mode 100644 src/JsonApiDotNetCore/Middleware/FixedQueryFeature.cs create mode 100644 src/JsonApiDotNetCore/Middleware/FixedQueryHelpers.cs diff --git a/src/JsonApiDotNetCore/Middleware/FixedQueryFeature.cs b/src/JsonApiDotNetCore/Middleware/FixedQueryFeature.cs new file mode 100644 index 0000000000..9ce2da0489 --- /dev/null +++ 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 +{ + /// + /// Replacement implementation for the ASP.NET built-in , to workaround bug https://github.com/dotnet/aspnetcore/issues/33394. + /// This is identical to the built-in version, except it calls . + /// + 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 NullRequestFeature = _ => null; + + private FeatureReferences _features; + + private string _original; + private IQueryCollection _parsedValues; + + private IHttpRequestFeature HttpRequestFeature => _features.Fetch(ref _features.Cache, NullRequestFeature); + + /// + 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 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; + } + } + } + } + + /// + /// Initializes a new instance of . + /// + /// + /// The to initialize. + /// + 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 new file mode 100644 index 0000000000..621aca493d --- /dev/null +++ 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 +{ + /// + /// Replacement implementation for the ASP.NET built-in , 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. + /// + internal static class FixedQueryHelpers + { + /// + /// Parse a query string into its component key and value parts. + /// + /// + /// The raw query string value, with or without the leading '?'. + /// + /// + /// A collection of parsed keys and values, null if there are no entries. + /// + public static Dictionary 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 index bfb486ed68..6d55598a81 100644 --- 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(new FixedQueryFeature(httpContext.Features)); + await _next(httpContext); }