Allow GraphQL to filter on nested property #1868

antograssiot · 2018-04-15T07:56:49Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1714, #1867
License	MIT
Doc PR

Possible replacement for #1743 without updating the filter but just the way filters are passed to the data provider.

ping @alanpoulain @IonBazan

fixes api-platform#1714, api-platform#1867

IonBazan

We should remember that it can produce a conflict when someone creates a filter with such name.

IonBazan · 2018-04-15T08:38:04Z

Please also add a test for order and date filter to make sure it's ok.

antograssiot · 2018-04-15T09:52:09Z

I will add it for date, ordering on nested property doesn't make much sense but I might be missing the point ?

IonBazan · 2018-04-15T09:58:16Z

Consider fetching posts ordered by author's rating (stars).

alanpoulain · 2018-04-15T10:06:18Z

I don't really like this way. The issue will be too obscure for an user if it used a custom filter with an underscore. I would prefer the other method.
Edit: Sorry I didn't see you add an alternative like the other method.

antograssiot · 2018-04-15T10:08:46Z

Sure I figured out just after commenting. this patch will need more work because the keys of value would need to be cleaned when they are arrays and for the key order. I'll have a look tonight maybe.

antograssiot · 2018-04-15T11:00:56Z

So should I spend more time on this to get also the nested filed ordering to work @alanpoulain or should I close it of the other PR is a prefered solution and should be extented for OrderFilter ?

alanpoulain · 2018-04-15T13:20:11Z

IMO, since it's a GraphQL-related issue, this solution is better.

antograssiot · 2018-04-15T13:33:27Z

I've added a naive workaround so ordering on nesting property value would work now.
What's your opinion ?

IonBazan · 2018-04-15T13:41:10Z

I wonder if it's possible to use nested property filters like:
dummies(relatedDummy: {name: "foo"}) and dummies(order: {relatedDummy: {name: "foo"}}) because in your solution, filters named: related_name and related.name are ambiguous.

antograssiot · 2018-04-15T13:48:13Z

@IonBazan I don't know much about the graphQL part yet, but at least the solution is the same for all filters used in graphQL context without changing the way it works for standard Rest API, that's why I went with that.
Whatever the solution will be at the end, it should be properly documented I think...

antograssiot · 2018-04-15T13:49:49Z

I do understand that for people using GraphQL only, it might be tricky to define filters using the dot notation and use and underscored notation when using it but I don't really see any other better solution for now...

alanpoulain · 2018-04-15T13:48:20Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

+    {
+        $filters = $args;
+        foreach ($filters as $name => &$value) {
+            if ($name === $this->orderParameterName && \is_array($value)) {


Why not generalizing it for all filters (i.e. only verifying if $value is an array)?

My first idea was that this is not necessary fo all filters (like DateFilter) for example and wanted to keep the modification as small as possible... but on a second thought, I don't see any issue in generalizing it.

And it simplifies the code 😉

alanpoulain · 2018-04-15T13:50:31Z

features/graphql/filters.feature

+    When I add "Accept" header equal to "application/hal+json"
+    And I send a "GET" request to "/dummies?relatedDummies.name=RelatedDummy31"
+    Then the response status code should be 200
+    And the response should be in JSON


This test shouldn't be here.

alanpoulain · 2018-04-15T13:51:12Z

features/graphql/filters.feature

+    When I send the following GraphQL request:
+    """
+    {
+      dummies(order:{relatedDummy_name: "DESC"}) {


Please add a space after order:.

alanpoulain · 2018-04-15T13:52:48Z

src/GraphQl/Type/SchemaBuilder.php

    {
        foreach ($args as $key => $value) {
+            if ($normalizeKeys && (strpos($key, '.'))) {


Same, why not generalizing it?

alanpoulain · 2018-04-15T13:55:53Z

Maybe we could use another character than the underscore to avoid confusion?

IonBazan · 2018-04-15T14:09:23Z

@alanpoulain I would go for dummies?relatedDummy[anotherRelatedDummy][name]=foo for REST and dummies(relatedDummy: {anotherRelatedDummy: {name: "foo"}}) for GraphQL instead of dots and underscores because it ensures consistency between the APIs but it is also a BC break.

alanpoulain · 2018-04-15T14:15:55Z

Seems fine to me. I think there is no BC break for now because GraphQL is treated as an experimental feature. It's not really clear enough for end-users though.

IonBazan · 2018-04-15T14:22:50Z

@alanpoulain actually it would be a BC break for REST. Currently, nested property filters are built like: relatedDummy.name - not relatedDummy[name].

alanpoulain · 2018-04-15T14:37:21Z

I think I get it. You want to avoid characters replacement by having the same kind of notation for the REST part and the GraphQL one?

IonBazan · 2018-04-15T14:46:31Z

@alanpoulain Exactly, but I'm not sure if there are any drawbacks of this solution. This would require a total filters refactor and a lot of testing. I think the solution proposed by @antograssiot is good enough for now.

IonBazan · 2018-04-15T18:49:16Z

features/graphql/filters.feature

+  Scenario: Retrieve a collection filtered using the related search filter
+    Given there are 1 dummy objects having each 2 relatedDummies
+    And there are 1 dummy objects having each 3 relatedDummies
+    When I add "Accept" header equal to "application/hal+json"


I think it shouldn't be here as we are testing GraphQL endpoint, not HAL.

antograssiot · 2018-04-15T19:02:15Z

all comments so far have been addressed.

alanpoulain · 2018-04-16T07:59:27Z

src/GraphQl/Type/SchemaBuilder.php

@@ -292,6 +292,9 @@ private function mergeFilterArgs(array $args, array $parsed, ResourceMetadata $r
    private function convertFilterArgsToTypes(array $args): array
    {
        foreach ($args as $key => $value) {
+            if (strpos($key, '.')) {
+                $args[str_replace('.', '_', $key)] = $value;
+            }


Please add a line break after.

alanpoulain · 2018-04-16T08:01:01Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

@@ -164,4 +164,22 @@ private function getSubresource(string $rootClass, array $rootResolvedFields, ar
            'collection' => $isCollection,
        ]);
    }
+
+    private function getNormalizedFilters($args)


Add :array at the end and array for the parameter.

alanpoulain · 2018-04-16T08:02:24Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

+    private function getNormalizedFilters($args)
+    {
+        $filters = $args;
+        foreach ($filters as $name => &$value) {


Please don't use references.

alanpoulain · 2018-04-16T08:12:13Z

src/GraphQl/Resolver/Factory/CollectionResolverFactory.php

+            }
+
+            if (strpos($name, '_')) {
+                // Gives a chance to relations/nested fields


Please add a final point.

antograssiot · 2018-04-16T09:04:16Z

done @alanpoulain

alanpoulain · 2018-04-16T09:34:27Z

src/GraphQl/Type/SchemaBuilder.php

@@ -292,6 +292,11 @@ private function mergeFilterArgs(array $args, array $parsed, ResourceMetadata $r
    private function convertFilterArgsToTypes(array $args): array
    {
        foreach ($args as $key => $value) {
+            if (strpos($key, '.')) {
+                // Declare relations/nested fields in a GraphQL compatible syntax.
+                $args[str_replace('.', '_', $key)] = $value;


Is this really working? Because we are not looping with the added value.

dunglas · 2018-04-20T09:54:27Z

Good one, thank you very much @antograssiot. Can you also open a doc PR? It looks important to have this mentioned in the documentation PR (especially for REST actually).

antograssiot · 2018-04-20T09:56:50Z

Sure, I'll prepare a doc update this weekend or next week @dunglas

* Allow GraphQL to filter on nested property fixes api-platform#1714, api-platform#1867 * Allow ordering on nested property values

Allow GraphQL to filter on nested property

76cfbf1

fixes api-platform#1714, api-platform#1867

IonBazan approved these changes Apr 15, 2018

View reviewed changes

alanpoulain reviewed Apr 15, 2018

View reviewed changes

antograssiot force-pushed the issue-1867 branch from 80e8b17 to 94e66e8 Compare April 15, 2018 17:14

IonBazan reviewed Apr 15, 2018

View reviewed changes

antograssiot force-pushed the issue-1867 branch 2 times, most recently from fee0d3f to 08f169d Compare April 15, 2018 19:01

alanpoulain requested changes Apr 16, 2018

View reviewed changes

antograssiot force-pushed the issue-1867 branch from 08f169d to 38b383a Compare April 16, 2018 09:01

alanpoulain reviewed Apr 16, 2018

View reviewed changes

Allow ordering on nested property values

78457bb

antograssiot force-pushed the issue-1867 branch from 38b383a to 78457bb Compare April 16, 2018 10:00

alanpoulain approved these changes Apr 16, 2018

View reviewed changes

dunglas merged commit c139586 into api-platform:2.2 Apr 20, 2018

antograssiot deleted the issue-1867 branch April 20, 2018 09:56

Simperfit mentioned this pull request Apr 20, 2018

fix property name in ContextAwareFilter to support GraphQL #1743

Closed

bunkerskyi mentioned this pull request Apr 21, 2018

GraphQL broke after some commit #1880

Closed

antograssiot mentioned this pull request Apr 24, 2018

Describe how to filter on nested properties with GraphQL api-platform/docs#472

Merged

teohhanhui mentioned this pull request May 2, 2018

[Do not merge] Revert merge master into 2.2 #1923

Closed

19 tasks

Uh oh!

Allow GraphQL to filter on nested property #1868

Allow GraphQL to filter on nested property #1868

Uh oh!

Conversation

antograssiot commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IonBazan left a comment

Choose a reason for hiding this comment

Uh oh!

IonBazan commented Apr 15, 2018

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

IonBazan commented Apr 15, 2018

Uh oh!

alanpoulain commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

antograssiot commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Apr 15, 2018

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

IonBazan commented Apr 15, 2018

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain commented Apr 15, 2018

Uh oh!

IonBazan commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Apr 15, 2018

Uh oh!

IonBazan commented Apr 15, 2018

Uh oh!

alanpoulain commented Apr 15, 2018

Uh oh!

IonBazan commented Apr 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antograssiot commented Apr 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antograssiot commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Apr 20, 2018

Uh oh!

antograssiot commented Apr 20, 2018

Uh oh!

antograssiot commented Apr 15, 2018 •

edited

Loading

alanpoulain commented Apr 15, 2018 •

edited

Loading

antograssiot commented Apr 15, 2018 •

edited

Loading

IonBazan commented Apr 15, 2018 •

edited

Loading