From 21c84f8375ecb2b3f56b0fa9d85a546b46d41b5d Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 20 Feb 2020 19:26:46 +0100 Subject: [PATCH 1/3] Includes query string parameters in top-level self link and paging links --- .../Builders/JsonApiApplicationBuilder.cs | 3 + .../Common/IRequestQueryStringAccessor.cs | 10 ++++ .../Common/RequestQueryStringAccessor.cs | 17 ++++++ .../Server/Builders/LinkBuilder.cs | 29 +++++++++- .../Acceptance/Spec/PagingTests.cs | 30 ++++++---- test/UnitTests/Builders/LinkBuilderTests.cs | 55 ++++++++++++------- 6 files changed, 111 insertions(+), 33 deletions(-) create mode 100644 src/JsonApiDotNetCore/QueryParameterServices/Common/IRequestQueryStringAccessor.cs create mode 100644 src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs diff --git a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs index c2ae694b64..03ab8811b5 100644 --- a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs +++ b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs @@ -21,6 +21,7 @@ using JsonApiDotNetCore.Serialization.Server; using Microsoft.Extensions.DependencyInjection.Extensions; using JsonApiDotNetCore.Extensions; +using JsonApiDotNetCore.QueryParameterServices.Common; namespace JsonApiDotNetCore.Builders { @@ -164,6 +165,8 @@ public void ConfigureServices() private void AddQueryParameterServices() { + _services.AddScoped(); + _services.AddScoped(); _services.AddScoped(); _services.AddScoped(); diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/IRequestQueryStringAccessor.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/IRequestQueryStringAccessor.cs new file mode 100644 index 0000000000..8ec146a292 --- /dev/null +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/IRequestQueryStringAccessor.cs @@ -0,0 +1,10 @@ +using Microsoft.AspNetCore.Http; + +namespace JsonApiDotNetCore.QueryParameterServices.Common +{ + public interface IRequestQueryStringAccessor + { + QueryString QueryString { get; } + IQueryCollection Query { get; } + } +} diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs new file mode 100644 index 0000000000..724bf815ba --- /dev/null +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs @@ -0,0 +1,17 @@ +using Microsoft.AspNetCore.Http; + +namespace JsonApiDotNetCore.QueryParameterServices.Common +{ + public class RequestQueryStringAccessor : IRequestQueryStringAccessor + { + private readonly IHttpContextAccessor _httpContextAccessor; + + public QueryString QueryString => _httpContextAccessor.HttpContext.Request.QueryString; + public IQueryCollection Query => _httpContextAccessor.HttpContext.Request.Query; + + public RequestQueryStringAccessor(IHttpContextAccessor httpContextAccessor) + { + _httpContextAccessor = httpContextAccessor; + } + } +} diff --git a/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs b/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs index 5d81048988..885980e727 100644 --- a/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs +++ b/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs @@ -1,3 +1,6 @@ +using System; +using System.Collections.Generic; +using System.Linq; using System.Text; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Internal; @@ -6,12 +9,15 @@ using JsonApiDotNetCore.Models; using JsonApiDotNetCore.Models.Links; using JsonApiDotNetCore.Query; +using JsonApiDotNetCore.QueryParameterServices.Common; +using Microsoft.AspNetCore.Http; namespace JsonApiDotNetCore.Serialization.Server.Builders { public class LinkBuilder : ILinkBuilder { private readonly IResourceContextProvider _provider; + private readonly IRequestQueryStringAccessor _queryStringAccessor; private readonly ILinksConfiguration _options; private readonly ICurrentRequest _currentRequest; private readonly IPageService _pageService; @@ -19,12 +25,14 @@ public class LinkBuilder : ILinkBuilder public LinkBuilder(ILinksConfiguration options, ICurrentRequest currentRequest, IPageService pageService, - IResourceContextProvider provider) + IResourceContextProvider provider, + IRequestQueryStringAccessor queryStringAccessor) { _options = options; _currentRequest = currentRequest; _pageService = pageService; _provider = provider; + _queryStringAccessor = queryStringAccessor; } /// @@ -101,6 +109,8 @@ private string GetSelfTopLevelLink(ResourceContext resourceContext) builder.Append(_currentRequest.RequestRelationship.PublicRelationshipName); } + builder.Append(_queryStringAccessor.QueryString.Value); + return builder.ToString(); } @@ -111,9 +121,24 @@ private string GetPageLink(ResourceContext resourceContext, int pageOffset, int pageOffset = -pageOffset; } - return $"{GetBasePath()}/{resourceContext.ResourceName}?page[size]={pageSize}&page[number]={pageOffset}"; + string queryString = BuildQueryString(parameters => + { + parameters["page[size]"] = pageSize.ToString(); + parameters["page[number]"] = pageOffset.ToString(); + }); + + return $"{GetBasePath()}/{resourceContext.ResourceName}" + queryString; } + private string BuildQueryString(Action> updateAction) + { + var parameters = _queryStringAccessor.Query.ToDictionary(pair => pair.Key, pair => pair.Value.ToString()); + updateAction(parameters); + string queryString = QueryString.Create(parameters).Value; + + queryString = queryString.Replace("%5B", "[").Replace("%5D", "]"); + return queryString; + } /// public ResourceLinks GetResourceLinks(string resourceName, string id) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/PagingTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/PagingTests.cs index 3ff55bf427..b84a561d35 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/PagingTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/PagingTests.cs @@ -3,10 +3,11 @@ using System.Net; using System.Threading.Tasks; using Bogus; +using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Models; using JsonApiDotNetCoreExample; using JsonApiDotNetCoreExample.Models; -using JsonApiDotNetCoreExampleTests.Helpers.Models; +using Microsoft.AspNetCore.Http; using Newtonsoft.Json; using Xunit; using Person = JsonApiDotNetCoreExample.Models.Person; @@ -78,7 +79,10 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu { // Arrange var totalCount = 20; - var person = new Person(); + var person = new Person + { + LastName = "&Ampersand" + }; var todoItems = _todoItemFaker.Generate(totalCount).ToList(); foreach (var todoItem in todoItems) @@ -88,11 +92,13 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu Context.TodoItems.AddRange(todoItems); Context.SaveChanges(); - string route = $"/api/v1/todoItems"; - if (pageNum != 1) - { - route += $"?page[size]=5&page[number]={pageNum}"; - } + var options = GetService(); + options.AllowCustomQueryParameters = true; + + string routePrefix = "/api/v1/todoItems?filter[owner.lastName]=" + WebUtility.UrlEncode(person.LastName) + + "&fields[owner]=firstName&include=owner&sort=ordinal&omitDefault=true&omitNull=true&foo=bar,baz"; + string route = pageNum != 1 ? routePrefix + $"&page[size]=5&page[number]={pageNum}" : routePrefix; + // Act var response = await Client.GetAsync(route); @@ -101,10 +107,10 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu var body = await response.Content.ReadAsStringAsync(); var links = JsonConvert.DeserializeObject(body).Links; - Assert.EndsWith($"/api/v1/todoItems?page[size]=5&page[number]={selfLink}", links.Self); + Assert.EndsWith($"{routePrefix}&page[size]=5&page[number]={selfLink}", links.Self); if (firstLink.HasValue) { - Assert.EndsWith($"/api/v1/todoItems?page[size]=5&page[number]={firstLink.Value}", links.First); + Assert.EndsWith($"{routePrefix}&page[size]=5&page[number]={firstLink.Value}", links.First); } else { @@ -113,7 +119,7 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu if (prevLink.HasValue) { - Assert.EndsWith($"/api/v1/todoItems?page[size]=5&page[number]={prevLink}", links.Prev); + Assert.EndsWith($"{routePrefix}&page[size]=5&page[number]={prevLink}", links.Prev); } else { @@ -122,7 +128,7 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu if (nextLink.HasValue) { - Assert.EndsWith($"/api/v1/todoItems?page[size]=5&page[number]={nextLink}", links.Next); + Assert.EndsWith($"{routePrefix}&page[size]=5&page[number]={nextLink}", links.Next); } else { @@ -131,7 +137,7 @@ public async Task Pagination_OnGivenPage_DisplaysCorrectTopLevelLinks(int pageNu if (lastLink.HasValue) { - Assert.EndsWith($"/api/v1/todoItems?page[size]=5&page[number]={lastLink}", links.Last); + Assert.EndsWith($"{routePrefix}&page[size]=5&page[number]={lastLink}", links.Last); } else { diff --git a/test/UnitTests/Builders/LinkBuilderTests.cs b/test/UnitTests/Builders/LinkBuilderTests.cs index 5408fbf809..65b62af774 100644 --- a/test/UnitTests/Builders/LinkBuilderTests.cs +++ b/test/UnitTests/Builders/LinkBuilderTests.cs @@ -1,4 +1,3 @@ -using System; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; @@ -10,7 +9,10 @@ using Moq; using Xunit; using JsonApiDotNetCore.Query; +using JsonApiDotNetCore.QueryParameterServices.Common; using JsonApiDotNetCore.Serialization.Server.Builders; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.WebUtilities; namespace UnitTests { @@ -18,10 +20,13 @@ public class LinkBuilderTests { private readonly IPageService _pageService; private readonly Mock _provider = new Mock(); + private readonly IRequestQueryStringAccessor _queryStringAccessor = new FakeRequestQueryStringAccessor("?foo=bar"); private const string _host = "http://www.example.com"; private const int _baseId = 123; private const string _relationshipName = "author"; - private const string _topSelf = "http://www.example.com/articles"; + private const string _topSelf = "http://www.example.com/articles?foo=bar"; + private const string _topResourceSelf = "http://www.example.com/articles/123?foo=bar"; + private const string _topRelatedSelf = "http://www.example.com/articles/123/author?foo=bar"; private const string _resourceSelf = "http://www.example.com/articles/123"; private const string _relSelf = "http://www.example.com/articles/123/relationships/author"; private const string _relRelated = "http://www.example.com/articles/123/author"; @@ -47,7 +52,7 @@ public void BuildResourceLinks_GlobalAndResourceConfiguration_ExpectedResult(Lin var config = GetConfiguration(resourceLinks: global); var primaryResource = GetResourceContext
(resourceLinks: resource); _provider.Setup(m => m.GetResourceContext("articles")).Returns(primaryResource); - var builder = new LinkBuilder(config, GetRequestManager(), null, _provider.Object); + var builder = new LinkBuilder(config, GetRequestManager(), null, _provider.Object, _queryStringAccessor); // Act var links = builder.GetResourceLinks("articles", _baseId.ToString()); @@ -95,7 +100,7 @@ public void BuildRelationshipLinks_GlobalResourceAndAttrConfiguration_ExpectedLi var config = GetConfiguration(relationshipLinks: global); var primaryResource = GetResourceContext
(relationshipLinks: resource); _provider.Setup(m => m.GetResourceContext(typeof(Article))).Returns(primaryResource); - var builder = new LinkBuilder(config, GetRequestManager(), null, _provider.Object); + var builder = new LinkBuilder(config, GetRequestManager(), null, _provider.Object, _queryStringAccessor); var attr = new HasOneAttribute(links: relationship) { RightType = typeof(Author), PublicRelationshipName = "author" }; // Act @@ -134,14 +139,14 @@ public void BuildRelationshipLinks_GlobalResourceAndAttrConfiguration_ExpectedLi [InlineData(Link.None, Link.Self, _topSelf, false)] [InlineData(Link.None, Link.Paging, null, true)] [InlineData(Link.None, Link.None, null, false)] - [InlineData(Link.All, Link.Self, _resourceSelf, false)] - [InlineData(Link.Self, Link.Self, _resourceSelf, false)] - [InlineData(Link.Paging, Link.Self, _resourceSelf, false)] - [InlineData(Link.None, Link.Self, _resourceSelf, false)] - [InlineData(Link.All, Link.Self, _relRelated, false)] - [InlineData(Link.Self, Link.Self, _relRelated, false)] - [InlineData(Link.Paging, Link.Self, _relRelated, false)] - [InlineData(Link.None, Link.Self, _relRelated, false)] + [InlineData(Link.All, Link.Self, _topResourceSelf, false)] + [InlineData(Link.Self, Link.Self, _topResourceSelf, false)] + [InlineData(Link.Paging, Link.Self, _topResourceSelf, false)] + [InlineData(Link.None, Link.Self, _topResourceSelf, false)] + [InlineData(Link.All, Link.Self, _topRelatedSelf, false)] + [InlineData(Link.Self, Link.Self, _topRelatedSelf, false)] + [InlineData(Link.Paging, Link.Self, _topRelatedSelf, false)] + [InlineData(Link.None, Link.Self, _topRelatedSelf, false)] public void BuildTopLevelLinks_GlobalAndResourceConfiguration_ExpectedLinks(Link global, Link resource, string expectedSelfLink, @@ -153,10 +158,10 @@ public void BuildTopLevelLinks_GlobalAndResourceConfiguration_ExpectedLinks(Link _provider.Setup(m => m.GetResourceContext
()).Returns(primaryResource); bool useBaseId = expectedSelfLink != _topSelf; - string relationshipName = expectedSelfLink == _relRelated ? _relationshipName : null; + string relationshipName = expectedSelfLink == _topRelatedSelf ? _relationshipName : null; ICurrentRequest currentRequest = GetRequestManager(primaryResource, useBaseId, relationshipName); - var builder = new LinkBuilder(config, currentRequest, _pageService, _provider.Object); + var builder = new LinkBuilder(config, currentRequest, _pageService, _provider.Object, _queryStringAccessor); // Act var links = builder.GetTopLevelLinks(); @@ -176,11 +181,11 @@ private bool CheckLinks(TopLevelLinks links, bool pages, string expectedSelfLink { if (pages) { - return links.Self == $"{_host}/articles?page[size]=10&page[number]=2" - && links.First == $"{_host}/articles?page[size]=10&page[number]=1" - && links.Prev == $"{_host}/articles?page[size]=10&page[number]=1" - && links.Next == $"{_host}/articles?page[size]=10&page[number]=3" - && links.Last == $"{_host}/articles?page[size]=10&page[number]=3"; + return links.Self == $"{_host}/articles?foo=bar&page[size]=10&page[number]=2" + && links.First == $"{_host}/articles?foo=bar&page[size]=10&page[number]=1" + && links.Prev == $"{_host}/articles?foo=bar&page[size]=10&page[number]=1" + && links.Next == $"{_host}/articles?foo=bar&page[size]=10&page[number]=3" + && links.Last == $"{_host}/articles?foo=bar&page[size]=10&page[number]=3"; } return links.Self == expectedSelfLink && links.First == null && links.Prev == null && links.Next == null && links.Last == null; @@ -230,5 +235,17 @@ private ResourceContext GetResourceContext(Link resourceLinks = Link. ResourceName = typeof(TResource).Name.Dasherize() + "s" }; } + + private sealed class FakeRequestQueryStringAccessor : IRequestQueryStringAccessor + { + public QueryString QueryString { get; } + public IQueryCollection Query { get; } + + public FakeRequestQueryStringAccessor(string queryString) + { + QueryString = new QueryString(queryString); + Query = new QueryCollection(QueryHelpers.ParseQuery(queryString)); + } + } } } From 55421d6c9de96e4f91b8be5f0a17b835a4a9b3b0 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Wed, 25 Mar 2020 11:52:59 +0100 Subject: [PATCH 2/3] Review feedback: rename QueryParameterDiscovery to QueryParameterParser --- benchmarks/Query/QueryParserBenchmarks.cs | 22 +++++++++---------- .../Builders/JsonApiApplicationBuilder.cs | 2 +- .../Middleware/QueryParameterFilter.cs | 4 ++-- .../Common/IQueryParameterParser.cs | 2 +- .../Common/QueryParameterParser.cs | 4 ++-- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/benchmarks/Query/QueryParserBenchmarks.cs b/benchmarks/Query/QueryParserBenchmarks.cs index 6dd7e509c1..5efde23f3e 100644 --- a/benchmarks/Query/QueryParserBenchmarks.cs +++ b/benchmarks/Query/QueryParserBenchmarks.cs @@ -14,8 +14,8 @@ namespace Benchmarks.Query [MarkdownExporter, SimpleJob(launchCount: 3, warmupCount: 10, targetCount: 20), MemoryDiagnoser] public class QueryParserBenchmarks { - private readonly QueryParameterDiscovery _queryParameterDiscoveryForSort; - private readonly QueryParameterDiscovery _queryParameterDiscoveryForAll; + private readonly QueryParameterParser _queryParameterParserForSort; + private readonly QueryParameterParser _queryParameterParserForAll; public QueryParserBenchmarks() { @@ -27,11 +27,11 @@ public QueryParserBenchmarks() IResourceDefinitionProvider resourceDefinitionProvider = DependencyFactory.CreateResourceDefinitionProvider(resourceGraph); - _queryParameterDiscoveryForSort = CreateQueryParameterDiscoveryForSort(resourceGraph, currentRequest, resourceDefinitionProvider, options); - _queryParameterDiscoveryForAll = CreateQueryParameterDiscoveryForAll(resourceGraph, currentRequest, resourceDefinitionProvider, options); + _queryParameterParserForSort = CreateQueryParameterDiscoveryForSort(resourceGraph, currentRequest, resourceDefinitionProvider, options); + _queryParameterParserForAll = CreateQueryParameterDiscoveryForAll(resourceGraph, currentRequest, resourceDefinitionProvider, options); } - private static QueryParameterDiscovery CreateQueryParameterDiscoveryForSort(IResourceGraph resourceGraph, + private static QueryParameterParser CreateQueryParameterDiscoveryForSort(IResourceGraph resourceGraph, CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, IJsonApiOptions options) { ISortService sortService = new SortService(resourceDefinitionProvider, resourceGraph, currentRequest); @@ -41,10 +41,10 @@ private static QueryParameterDiscovery CreateQueryParameterDiscoveryForSort(IRes sortService }; - return new QueryParameterDiscovery(options, queryServices); + return new QueryParameterParser(options, queryServices); } - private static QueryParameterDiscovery CreateQueryParameterDiscoveryForAll(IResourceGraph resourceGraph, + private static QueryParameterParser CreateQueryParameterDiscoveryForAll(IResourceGraph resourceGraph, CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, IJsonApiOptions options) { IIncludeService includeService = new IncludeService(resourceGraph, currentRequest); @@ -61,11 +61,11 @@ private static QueryParameterDiscovery CreateQueryParameterDiscoveryForAll(IReso omitNullService }; - return new QueryParameterDiscovery(options, queryServices); + return new QueryParameterParser(options, queryServices); } [Benchmark] - public void AscendingSort() => _queryParameterDiscoveryForSort.Parse(new QueryCollection( + public void AscendingSort() => _queryParameterParserForSort.Parse(new QueryCollection( new Dictionary { {"sort", BenchmarkResourcePublicNames.NameAttr} @@ -73,7 +73,7 @@ public void AscendingSort() => _queryParameterDiscoveryForSort.Parse(new QueryCo ), null); [Benchmark] - public void DescendingSort() => _queryParameterDiscoveryForSort.Parse(new QueryCollection( + public void DescendingSort() => _queryParameterParserForSort.Parse(new QueryCollection( new Dictionary { {"sort", $"-{BenchmarkResourcePublicNames.NameAttr}"} @@ -81,7 +81,7 @@ public void DescendingSort() => _queryParameterDiscoveryForSort.Parse(new QueryC ), null); [Benchmark] - public void ComplexQuery() => Run(100, () => _queryParameterDiscoveryForAll.Parse(new QueryCollection( + public void ComplexQuery() => Run(100, () => _queryParameterParserForAll.Parse(new QueryCollection( new Dictionary { {$"filter[{BenchmarkResourcePublicNames.NameAttr}]", new StringValues(new[] {"abc", "eq:abc"})}, diff --git a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs index f3169cb2c9..eb4f84bc22 100644 --- a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs +++ b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs @@ -148,7 +148,7 @@ public void ConfigureServices() _services.AddScoped(); _services.AddScoped(); _services.AddScoped(typeof(RepositoryRelationshipUpdateHelper<>)); - _services.AddScoped(); + _services.AddScoped(); _services.AddScoped(); _services.AddScoped(); _services.AddScoped(); diff --git a/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs b/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs index 863bf7ad3a..ec089e365f 100644 --- a/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs +++ b/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs @@ -8,8 +8,8 @@ namespace JsonApiDotNetCore.Middleware { public sealed class QueryParameterActionFilter : IAsyncActionFilter, IQueryParameterActionFilter { - private readonly IQueryParameterDiscovery _queryParser; - public QueryParameterActionFilter(IQueryParameterDiscovery queryParser) => _queryParser = queryParser; + private readonly IQueryParameterParser _queryParser; + public QueryParameterActionFilter(IQueryParameterParser queryParser) => _queryParser = queryParser; public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next) { diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs index 9a0c0b49a5..66df4a04da 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs @@ -8,7 +8,7 @@ namespace JsonApiDotNetCore.Services /// Responsible for populating the various service implementations of /// . /// - public interface IQueryParameterDiscovery + public interface IQueryParameterParser { void Parse(IQueryCollection query, DisableQueryAttribute disabledQuery = null); } diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs index 1e397afbf7..917a604499 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs @@ -9,12 +9,12 @@ namespace JsonApiDotNetCore.Services { /// - public class QueryParameterDiscovery : IQueryParameterDiscovery + public class QueryParameterParser : IQueryParameterParser { private readonly IJsonApiOptions _options; private readonly IEnumerable _queryServices; - public QueryParameterDiscovery(IJsonApiOptions options, IEnumerable queryServices) + public QueryParameterParser(IJsonApiOptions options, IEnumerable queryServices) { _options = options; _queryServices = queryServices; From 0edc25b2c1a914801651994aaff5a882c274e7ae Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Wed, 25 Mar 2020 12:27:26 +0100 Subject: [PATCH 3/3] Review feedback: Make QueryParameterParser use IRequestQueryStringAccessor; make RequestQueryStringAccessor internal and register as singleton --- benchmarks/Query/QueryParserBenchmarks.cs | 73 +++++++++++-------- .../Builders/JsonApiApplicationBuilder.cs | 4 +- .../Middleware/QueryParameterFilter.cs | 2 +- .../Common/IQueryParameterParser.cs | 3 +- .../Common/QueryParameterParser.cs | 12 +-- .../Common/RequestQueryStringAccessor.cs | 2 +- 6 files changed, 56 insertions(+), 40 deletions(-) diff --git a/benchmarks/Query/QueryParserBenchmarks.cs b/benchmarks/Query/QueryParserBenchmarks.cs index 5efde23f3e..336e758843 100644 --- a/benchmarks/Query/QueryParserBenchmarks.cs +++ b/benchmarks/Query/QueryParserBenchmarks.cs @@ -5,15 +5,17 @@ using JsonApiDotNetCore.Internal.Contracts; using JsonApiDotNetCore.Managers; using JsonApiDotNetCore.Query; +using JsonApiDotNetCore.QueryParameterServices.Common; using JsonApiDotNetCore.Services; using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Primitives; +using Microsoft.AspNetCore.WebUtilities; namespace Benchmarks.Query { [MarkdownExporter, SimpleJob(launchCount: 3, warmupCount: 10, targetCount: 20), MemoryDiagnoser] public class QueryParserBenchmarks { + private readonly FakeRequestQueryStringAccessor _queryStringAccessor = new FakeRequestQueryStringAccessor(); private readonly QueryParameterParser _queryParameterParserForSort; private readonly QueryParameterParser _queryParameterParserForAll; @@ -27,12 +29,13 @@ public QueryParserBenchmarks() IResourceDefinitionProvider resourceDefinitionProvider = DependencyFactory.CreateResourceDefinitionProvider(resourceGraph); - _queryParameterParserForSort = CreateQueryParameterDiscoveryForSort(resourceGraph, currentRequest, resourceDefinitionProvider, options); - _queryParameterParserForAll = CreateQueryParameterDiscoveryForAll(resourceGraph, currentRequest, resourceDefinitionProvider, options); + _queryParameterParserForSort = CreateQueryParameterDiscoveryForSort(resourceGraph, currentRequest, resourceDefinitionProvider, options, _queryStringAccessor); + _queryParameterParserForAll = CreateQueryParameterDiscoveryForAll(resourceGraph, currentRequest, resourceDefinitionProvider, options, _queryStringAccessor); } private static QueryParameterParser CreateQueryParameterDiscoveryForSort(IResourceGraph resourceGraph, - CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, IJsonApiOptions options) + CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, + IJsonApiOptions options, FakeRequestQueryStringAccessor queryStringAccessor) { ISortService sortService = new SortService(resourceDefinitionProvider, resourceGraph, currentRequest); @@ -41,11 +44,12 @@ private static QueryParameterParser CreateQueryParameterDiscoveryForSort(IResour sortService }; - return new QueryParameterParser(options, queryServices); + return new QueryParameterParser(options, queryStringAccessor, queryServices); } private static QueryParameterParser CreateQueryParameterDiscoveryForAll(IResourceGraph resourceGraph, - CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, IJsonApiOptions options) + CurrentRequest currentRequest, IResourceDefinitionProvider resourceDefinitionProvider, + IJsonApiOptions options, FakeRequestQueryStringAccessor queryStringAccessor) { IIncludeService includeService = new IncludeService(resourceGraph, currentRequest); IFilterService filterService = new FilterService(resourceDefinitionProvider, resourceGraph, currentRequest); @@ -61,40 +65,51 @@ private static QueryParameterParser CreateQueryParameterDiscoveryForAll(IResourc omitNullService }; - return new QueryParameterParser(options, queryServices); + return new QueryParameterParser(options, queryStringAccessor, queryServices); } [Benchmark] - public void AscendingSort() => _queryParameterParserForSort.Parse(new QueryCollection( - new Dictionary - { - {"sort", BenchmarkResourcePublicNames.NameAttr} - } - ), null); + public void AscendingSort() + { + var queryString = $"?sort={BenchmarkResourcePublicNames.NameAttr}"; + + _queryStringAccessor.SetQueryString(queryString); + _queryParameterParserForSort.Parse(null); + } [Benchmark] - public void DescendingSort() => _queryParameterParserForSort.Parse(new QueryCollection( - new Dictionary - { - {"sort", $"-{BenchmarkResourcePublicNames.NameAttr}"} - } - ), null); + public void DescendingSort() + { + var queryString = $"?sort=-{BenchmarkResourcePublicNames.NameAttr}"; + + _queryStringAccessor.SetQueryString(queryString); + _queryParameterParserForSort.Parse(null); + } [Benchmark] - public void ComplexQuery() => Run(100, () => _queryParameterParserForAll.Parse(new QueryCollection( - new Dictionary - { - {$"filter[{BenchmarkResourcePublicNames.NameAttr}]", new StringValues(new[] {"abc", "eq:abc"})}, - {"sort", $"-{BenchmarkResourcePublicNames.NameAttr}"}, - {"include", "child"}, - {"page[size]", "1"}, - {"fields", BenchmarkResourcePublicNames.NameAttr} - } - ), null)); + public void ComplexQuery() => Run(100, () => + { + var queryString = $"?filter[{BenchmarkResourcePublicNames.NameAttr}]=abc,eq:abc&sort=-{BenchmarkResourcePublicNames.NameAttr}&include=child&page[size]=1&fields={BenchmarkResourcePublicNames.NameAttr}"; + + _queryStringAccessor.SetQueryString(queryString); + _queryParameterParserForAll.Parse(null); + }); private void Run(int iterations, Action action) { for (int i = 0; i < iterations; i++) action(); } + + private sealed class FakeRequestQueryStringAccessor : IRequestQueryStringAccessor + { + public QueryString QueryString { get; private set; } + public IQueryCollection Query { get; private set; } + + public void SetQueryString(string queryString) + { + QueryString = new QueryString(queryString); + Query = new QueryCollection(QueryHelpers.ParseQuery(queryString)); + } + } } } diff --git a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs index eb4f84bc22..2b7b023deb 100644 --- a/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs +++ b/src/JsonApiDotNetCore/Builders/JsonApiApplicationBuilder.cs @@ -142,6 +142,8 @@ public void ConfigureServices() _services.AddSingleton(); _services.AddSingleton(resourceGraph); _services.AddSingleton(resourceGraph); + _services.AddSingleton(); + _services.AddScoped(); _services.AddScoped(); _services.AddScoped(); @@ -164,8 +166,6 @@ public void ConfigureServices() private void AddQueryParameterServices() { - _services.AddScoped(); - _services.AddScoped(); _services.AddScoped(); _services.AddScoped(); diff --git a/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs b/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs index ec089e365f..cdc233ae87 100644 --- a/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs +++ b/src/JsonApiDotNetCore/Middleware/QueryParameterFilter.cs @@ -16,7 +16,7 @@ public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionE // gets the DisableQueryAttribute if set on the controller that is targeted by the current request. DisableQueryAttribute disabledQuery = context.Controller.GetType().GetTypeInfo().GetCustomAttribute(typeof(DisableQueryAttribute)) as DisableQueryAttribute; - _queryParser.Parse(context.HttpContext.Request.Query, disabledQuery); + _queryParser.Parse(disabledQuery); await next(); } } diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs index 66df4a04da..edabd64edd 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/IQueryParameterParser.cs @@ -1,6 +1,5 @@ using JsonApiDotNetCore.Controllers; using JsonApiDotNetCore.Query; -using Microsoft.AspNetCore.Http; namespace JsonApiDotNetCore.Services { @@ -10,6 +9,6 @@ namespace JsonApiDotNetCore.Services /// public interface IQueryParameterParser { - void Parse(IQueryCollection query, DisableQueryAttribute disabledQuery = null); + void Parse(DisableQueryAttribute disabledQuery = null); } } diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs index 917a604499..fe3b830f69 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/QueryParameterParser.cs @@ -4,7 +4,7 @@ using JsonApiDotNetCore.Controllers; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Query; -using Microsoft.AspNetCore.Http; +using JsonApiDotNetCore.QueryParameterServices.Common; namespace JsonApiDotNetCore.Services { @@ -12,24 +12,26 @@ namespace JsonApiDotNetCore.Services public class QueryParameterParser : IQueryParameterParser { private readonly IJsonApiOptions _options; + private readonly IRequestQueryStringAccessor _queryStringAccessor; private readonly IEnumerable _queryServices; - public QueryParameterParser(IJsonApiOptions options, IEnumerable queryServices) + public QueryParameterParser(IJsonApiOptions options, IRequestQueryStringAccessor queryStringAccessor, IEnumerable queryServices) { _options = options; + _queryStringAccessor = queryStringAccessor; _queryServices = queryServices; } /// - /// For a query parameter in , calls + /// For a parameter in the query string of the request URL, calls /// the /// method of the corresponding service. /// - public virtual void Parse(IQueryCollection query, DisableQueryAttribute disabled) + public virtual void Parse(DisableQueryAttribute disabled) { var disabledQuery = disabled?.QueryParams; - foreach (var pair in query) + foreach (var pair in _queryStringAccessor.Query) { bool parsed = false; foreach (var service in _queryServices) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs b/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs index 724bf815ba..974e79bc5e 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/Common/RequestQueryStringAccessor.cs @@ -2,7 +2,7 @@ namespace JsonApiDotNetCore.QueryParameterServices.Common { - public class RequestQueryStringAccessor : IRequestQueryStringAccessor + internal sealed class RequestQueryStringAccessor : IRequestQueryStringAccessor { private readonly IHttpContextAccessor _httpContextAccessor;