Refactor query mapper #8

crevillo · 2019-01-26T14:25:55Z

Prof of concept here.
It refactors the SearchQueryMapper to allow the extension of those criterios without the need to remodify that class again and again.
Still working on it, but i guess pr is the way to know if i'm the right track :).

bdunogier

Very good start.

As discussed on Slack, though, it would be better to handle both criteria and sortclauses using QueryInputVisitor objects : they get both the value AND a Query object, and add whatever they're in charge of to the Query.

Can you give it a try ?

And last thing: please base your PR on the dev branch, as it has the renamed namespace EzSystems\EzPlatformGraphQL. It will be merged to master very soon anyway (probably today).

GraphQL/InputMapper/SearchQueryMapper.php

Resources/config/search_query_mappers.yml

crevillo · 2019-01-28T09:54:35Z

thanks for the comments. they all make sense. yes, i was not totally happy either, but i think i will be more happy once i try to work on your suggestions.
Talk soon.

bdunogier · 2019-01-28T10:20:18Z

Remember that we take an iterative approach. We can break BC as long as we haven't hit v1.0. The refactoring of this input mapper is necessary in any case, and the general direction is a good one. Keep it coming :)

One thing that may help designing good code, since you're into DDD these days, is to write PhpSpec tests for the input mapper & visitors. They make bad code difficult to test, and push you towards good design. It would be a good opportunity to add a new skill to your portfolio.

crevillo · 2019-01-29T09:22:41Z

Applied some of the comments in the pr and also in slack. so now we have

the query mapper uses a query builder
the query builder has an addCriterion method. Visitors will add criterions to the querybuilder and also can set SortBy.
the query Builder has also a buildQuery method that returns a Content Query.

Open to more suggestions! :)

bdunogier

I think we're getting there. We could go further by generating the definition of ContentSearchQuery from the registered visitors, as it would make this fully extensible.

My only concern is how this works out with #5.

bdunogier · 2019-02-01T15:11:48Z

src/DependencyInjection/Compiler/SearchQueryMappersPass.php

+
+class SearchQueryMappersPass implements CompilerPassInterface
+{
+    const ID = 'EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\SearchQueryMapper';


I would probably remove the constant, and use SearchQueryMapper::class in the if below. Even more so since you use it right after ;)

bdunogier · 2019-02-01T15:13:11Z

src/DependencyInjection/Compiler/SearchQueryMappersPass.php

+        foreach ($taggedServices as $id => $tags) {
+            foreach ($tags as $tag) {
+                if (isset($tag['inputKey'])) {
+                    $queryInputVisitors[$tag['inputKey']] = new Reference($id);


Note: the advantage of this is that a query input visitor can be overridden using the key. Useful.

bdunogier · 2019-02-01T15:16:19Z

src/Resources/config/query_input_visitors.yml

+services:
+    EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\ContentTypeIdentifier:
+        tags:
+            - {name: 'ezplatform_graphql.query_input_visitor', inputKey: 'ContentTypeIdentifier'}


We could think of basing the definition of the ContentSearchQuery on the defined inputKey.

What it implies:

add a DomainIterator that iterates over registered criterion input visitors

add a worker that adds a field to ContentSearchQuery for each of them

The only remaining question is where we get the type of each field from.

In this regard, we should keep in mind that we would ideally need an input-object (like ContentSearchQuery) that does NOT have Content Type based fields: when filtering on a given content-type, they can't be used (well, they can, but then we will search for content that is of two types, and we don't have that :)

bdunogier · 2019-02-01T15:19:19Z

src/GraphQL/InputMapper/Search/Criterion/ParentLocationId.php

+{
+    public function visit(QueryBuilder $queryBuilder, $value): void
+    {
+        $queryBuilder->addCriterion(new Query\Criterion\ParentLocationId($value));


Should we refactor all visitors that instanciate a Criterion object to a generic one ? It could take a criterion class as a constructor argument. We could remove a few duplicate files with this.

but Field and DateMetadata seems to work a bit different than the others, aren't they?

Indeed !

But you could at least remove ContentTypeId, ContentTypeIdentifier, ParentLocationId and Text. Furthermore, there are more criteria that work the same way, we could easily add them if we have a class for it.

BasicCriterionVisitor ? Standard ?

i don't know if i follow you here.

do you mean having a Standard criterion visitor tagged with allo those input keys?
my doubt is, how to instatiate this standard criterion and tell it to use one criterion or another depending on the inputkey ?

An example:

class BasicCriterionVisitor implements QueryInputVisitor { public function __construct($criterionClass) { $this->criterionClass = $criterionClass; } public function visit(QueryBuilder $queryBuilder, $value): void { $queryBuilder->addCriterion(new {$this->criterionClass}($value));

services: EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\ContentTypeId: class: EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\Basic arguments: $criterionClass: 'eZ\Publish\API\Repository\Values\Content\Query\Criterion\ContentTypeId' tags: - {name: 'ezplatform_graphql.query_input_visitor', inputKey: 'ContentTypeId'} EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\ContentTypeIdentifier: class: EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\Basic arguments: $criterionClass: 'eZ\Publish\API\Repository\Values\Content\Query\Criterion\ContentTypeIdentifier' tags: - {name: 'ezplatform_graphql.query_input_visitor', inputKey: 'ContentTypeIdentifier'} EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\ParentLocationId: class: EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\Criterion\Basic arguments: $criterionClass: 'eZ\Publish\API\Repository\Values\Content\Query\Criterion\ParentLocationId' tags: - {name: 'ezplatform_graphql.query_input_visitor', inputKey: 'ParentLocationId'}

Does it make more sense ?

yes, much more clear now. i thought you were trying to avoid all those service definitions, but you're only trying to avoid php classes :)
i'll work on this.

crevillo · 2019-02-04T21:30:54Z

created the Standard Criterion visitor and removed the unneded ones. i would need some help with the spec now.

fix ident in yaml

crevillo · 2019-02-24T11:13:38Z

added spec for the query builder and modified the searchQueryMapper spec too.

bdunogier

We are getting there, I think :)

bdunogier · 2019-02-07T15:59:47Z

src/GraphQL/InputMapper/Search/SortBy.php

+
+use EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\QueryBuilder;
+
+class SortBy implements QueryInputVisitor


Would it make sense to refactor this with a SortClauseVisitor, like you did for Criteria ? There are other sort clauses we may wanna support that won't work with this generic code.

but currently SortBy are at "the same level" as criterion when building queries with the graphql syntax, right? so, probably, sortby should still be a query input visitor and then we could have sort clauses visitors that could add sortclauses to the query builder?

@bdunogier i think it makes sense what you suggest, but let me ask if i'm in the right direction. currently we have this in the Search.types.yml file.

SortByOptions: type: enum config: values: _contentId: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\ContentId' description: "Sort by content id" _name: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\ContentName' description: "Sort by content name" _dateModified: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\DateModified' description: "Sort by last modification date" _datePublished: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\DatePublished' description: "Sort by initial publication date" _sectionIdentifier: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\SectionIdentifier' description: "Sort by content section identifier" _sectionName: value: '\eZ\Publish\API\Repository\Values\Content\Query\SortClause\SectionName' description: "Sort by section name" _desc: value: 'descending' description: "Reverse the previous sorting option"

Do you mean changes definition there to have it more similar to the ContentSearchQuery one? n (i mean, not map to class directly in the yml)

GraphQL/InputMapper/Search/Criterion/DateMetadata.php

GraphQL/InputMapper/Search/Criterion/Text.php

src/GraphQL/InputMapper/Search/Criterion/Standard.php

src/GraphQL/InputMapper/Search/Criterion/DateMetadata.php

src/GraphQL/InputMapper/Search/QueryBuilder.php

bdunogier · 2019-02-26T12:19:00Z

src/GraphQL/InputMapper/Search/SortBy.php

+    public function visit(QueryBuilder $queryBuilder, $value): void
+    {
+        $queryBuilder->setSortBy(array_map(
+            function ($sortClauseClass) {


Maybe we could move that method to a class method ? It made sense to have it inline in the original implementation, but now we can make this a bit cleaner.

…m-graphql into refactor-query-mapper

bdunogier requested changes Jan 28, 2019

View reviewed changes

crevillo added 8 commits January 28, 2019 18:58

add compiler pass for search query mappers

0dbc81e

add dedicated file for search query mappers and load it

ffa7bf7

define interface for searchCriterion and first of them

cb3f444

create rest of the current available search criterions

83bf501

rename method name

8f0ad91

add method to check if inputField can be mapped to a criteria

4639965

extract sortByPart logic to dedicate method

4b19c66

update namespaces after conflict resolution

2279ce2

crevillo changed the base branch from master to dev January 28, 2019 18:06

bdunogier reviewed Feb 1, 2019

View reviewed changes

crevillo added 11 commits February 4, 2019 22:36

add dedicated file for search query mappers and load it

a0925a7

define interface for searchCriterion and first of them

6d9b245

create rest of the current available search criterions

75ec18d

rename method name

594c2a0

updated namespaces after conflict

832969c

some typehints

8ed08be

rename file with the visitors

eea3c5c

add querybuilder and adapt all the visitors to the new interface

05aa445

use "class" constant instead of string

cd4cb88

remove unneded var

a0b555e

add basic criterion visitor

4494d2a

fix ident in yaml

crevillo force-pushed the refactor-query-mapper branch from bd4572b to 4494d2a Compare February 4, 2019 21:39

spec for querybuilder and searchQueryMapper

e6fab24

crevillo added 2 commits February 25, 2019 12:35

deleted unneded folder

7184393

cs

37dcd75

bdunogier requested changes Feb 26, 2019

View reviewed changes

crevillo added 5 commits February 26, 2019 19:43

spec for querybuilder and searchQueryMapper

acc7834

wip

7b22af1

wip

d4a0b70

Merge branch 'refactor-query-mapper' of github.com:crevillo/ezplatfor…

4cf468b

…m-graphql into refactor-query-mapper

refactor to have visitors for sort clauses

af2fbf6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor query mapper #8

Refactor query mapper #8

crevillo commented Jan 26, 2019

bdunogier left a comment

crevillo commented Jan 28, 2019

bdunogier commented Jan 28, 2019

crevillo commented Jan 29, 2019

bdunogier left a comment

bdunogier Feb 1, 2019

bdunogier Feb 1, 2019

bdunogier Feb 1, 2019

bdunogier Feb 1, 2019

bdunogier Feb 1, 2019

crevillo Feb 1, 2019

bdunogier Feb 1, 2019

crevillo Feb 2, 2019

bdunogier Feb 4, 2019

crevillo Feb 4, 2019

crevillo commented Feb 4, 2019

crevillo commented Feb 24, 2019

bdunogier left a comment

bdunogier Feb 7, 2019

crevillo Feb 26, 2019 •

edited

Loading

crevillo Mar 3, 2019

bdunogier Feb 26, 2019


		use EzSystems\EzPlatformGraphQL\GraphQL\InputMapper\Search\QueryBuilder;

		class SortBy implements QueryInputVisitor

Refactor query mapper #8

Are you sure you want to change the base?

Refactor query mapper #8

Conversation

crevillo commented Jan 26, 2019

bdunogier left a comment

Choose a reason for hiding this comment

crevillo commented Jan 28, 2019

bdunogier commented Jan 28, 2019

crevillo commented Jan 29, 2019

bdunogier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crevillo commented Feb 4, 2019

crevillo commented Feb 24, 2019

bdunogier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crevillo Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crevillo Feb 26, 2019 •

edited

Loading