Skip to content

Query Parameter Services #574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Oct 15, 2019
Merged

Query Parameter Services #574

merged 17 commits into from
Oct 15, 2019

Conversation

maurei
Copy link
Member

@maurei maurei commented Oct 15, 2019

Closes #562

This PR contains a rework of the internal handling of query parameters. Apart from refactoring, cleaning up and adding unit tests, it contains the following major changes:

  • Decoupled query parameter services
  • Migrated responsibilities
  • Easier extensibility

These are now discussed below

Decoupled query parameter services

Every supported url query parameter now has an associated dedicated service for parsing it and internally exposing it.

Query Parameter Service Occurence in URL Domain
IFilterService ?filter[article.title]=title filtering the resultset
IIncludeService ?include=article.author including related data
IPageService ?page[size]=10&page[number]=3 pagination of the resultset
ISortService ?sort=-title sorting the resultset
ISparseFieldsService ?fields[article]=title,summary sparse field selection
IOmitDefaultService ?omitDefault=true omitting default values from the serialization result, eg guid-value": "00000000-0000-0000-0000-000000000000"
IOmitNullService ?omitNull=false omitting null values from the serialization result

Throughout the framework, each of these services are now injected only in the places where they are needed. For example: the serialization layer does no longer have access to the filter, sort, page queries.

Migrated responsibilities

As mentioned in the previous part, these dedicated services are responsible for parsing and internally exposing the query parameter. This implies that the following migrations of responsibilities have been introduced

  • the previously existing IQueryParser service is no longer responsible for parsing the url query parameters. The parsing logic for each query has been moved to it's dedicated service (through the implementation of the IQueryParameter interface).
    • eg only the IFilterService should know how to parse a filter[X]=Y query string
  • IQueryParser is now IQueryParameterDiscovery which is only responsible for discovering the registered query parameter services and providing them with the appropriate part of the URL query string
    • eg it calles the IFilterServices.Parse method with the filter[X]=Y part of the query string.
  • the repository layer used to be responsible for validating parsed values of query parameters. This responsibility too has been moved to the dedicated query parameter service.
    • eg the Include method used to be called with a list of strings (like "article.author.invalid-relationship"), and this list would then be validated and converted into the RelationshipsAttribute that is actually reflected by that relationship chian. This type of validation now happens directly upon parsing the query parameters in the middleware

Better extensibility

To register a custom query parameter, all one needs to do is register an implementation of IQueryParameterService. See the Query Parameter Services wiki.

{
Expression body;
switch (operation)
{
case FilterOperations.eq:
case FilterOperation.eq:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency and best practise: use singular words for enums

@@ -197,12 +195,12 @@ private static void AddMvcOptions(MvcOptions options, JsonApiOptions config)
services.AddScoped<IJsonApiReader, JsonApiReader>();
services.AddScoped<IGenericProcessorFactory, GenericProcessorFactory>();
services.AddScoped(typeof(GenericProcessor<>));
services.AddScoped<IQueryAccessor, QueryAccessor>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are fully deprecated in their old forms

/// <summary>
/// A context class that provides extra meta data for a <see cref="TQuery"/>
/// that is used when applying url queries internally.
/// </summary>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an entirely new class, merely a refactor of the previously RelatedAttrFilterQuery and AttributeAttrFilterQuery

class CurrentRequest : ICurrentRequest
{
private ContextEntity _contextEntity;
public string BasePath { get; set; }
public List<string> IncludedRelationships { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed things from former request manager that are no longer referenced

@@ -83,9 +80,7 @@ private string GetSelfTopLevelLink(string resourceName)

private string GetPageLink(ContextEntity primaryResource, int pageOffset, int pageSize)
{
var filterQueryComposer = new QueryComposer();
var filters = filterQueryComposer.Compose(_currentRequest);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this as it is incomplete and untested.

@maurei maurei requested a review from wisepotato October 15, 2019 11:43
@wisepotato
Copy link
Contributor

We should consider adding Query in each of the service names.

@wisepotato
Copy link
Contributor

wiki is clear however it looks like i can only insert one extra paramservice, is this true? how do i add two? would i have:

services.AddScoped<IQueryParameterService, TeapotService>();
services.AddScoped<IQueryParameterService, OtherTeaPotService>();

?

@wisepotato wisepotato merged commit 8d90ac9 into dev-v4 Oct 15, 2019
@wisepotato wisepotato deleted the feat/queryparams branch October 15, 2019 13:20
@maurei maurei mentioned this pull request Oct 23, 2019
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants