Order filter. #43

theofidry · 2015-04-30T16:36:34Z

Hi @dunglas and @sroze , as promised the order filter rebased!

I added Behat features and modified fixtures to properly test my order filter.

Syntax

The URL query syntax is the same as before:

uri?order[property]=<ASC|DESC>

With property the name of the property on which the ordering is done, and the ordering value is case insensitive.

Of course it is possible to add multiple ordering filters and in this case the order will be done in the order of declaration.

Warning1

Issue:

A case of conflict may be if the user enable a where filter on an order property. If he does so, only the last declaration will be taken into account... I guess this is a reason why I've seen some API using the syntax filter[where][property]=value and filter[order]=propertyName <ASC|DESC> as for instance StrongLoop does. At this point I think this conflict case may be rare enough to ignore it up to you to decide!

Update: Problem ignored. If there is any conflict the user can easily override the filter.

Warning2

Issue:

No check is done whether or not the property enabled exist or not in the entity. I do not know which behavior you judge better although I think a little check is better. Besides it makes more sense with the use of the blacklist (see description below). There is also the question of if an error should be displayed to the user if something odd is happening, cf issue #42.

Update: By default if the property does not exists, it is silently ignored even if has been declared in the configuration.

Declaration

The declaration is done as the following:

my_dummy_resource.filter.order:
    parent:    "api.doctrine.orm.order_filter"
    arguments: [ ["id", "name"] ]

my_dummy_resource:
    parent:    "api.resource"
    arguments: [ "Dunglas\ApiBundle\Tests\Behat\TestBundle\Entity\Dummy" ]
    calls:
         - method: "addFilter"
           arguments:
               -   "@my_dummy_resource.filter.order"
        tags:      [ { name: "api.resource" } ]

Which will enable the ordering filter for the properties id and name.

Blacklisting properties

Instead of declaring the list on which we enable the order filter, it is also possible to defined which properties on which the order filter is not enabled. In this case all property with a valid value and not in the blacklist may be passed. The declaration is done by adding the boolean value false to the arguments:

my_dummy_resource.filter.order:
    parent:    "api.doctrine.orm.order_filter"
    arguments: [ ["id", "name"] , false]

# or another way if lots of properties:
my_dummy_resource.filter.order:
    parent:    "api.doctrine.orm.order_filter"
    arguments:
        - ["id", "name"]
        - false

As you can guess the boolean value is set to true by default.

Code change/remarks

Added PHPUnit tests for unit testing OrderFilter
Added a separate behat feature file for function test on order filter on collection to keep the original feature on collection readable
The logic of the OrderFilter is done in a protected function applyFilter(). This way, if the user use another pattern for URL querying, he can change the way to retrieve values from it without having to change how the filters works, which I guess will be the main reason for which one will override this filter.

TODO

Make Behat tests pass
Add documentation
Rebase to clean up commits

sroze · 2015-05-01T08:21:16Z

Doctrine/Orm/OrderFilter.php

+
+    /**
+     * @param array   $properties List of property names on which the filter will be enabled/disabled.
+     * @param boolean $type       If true properties are passed as a whitelist and as a blacklist otherwise.


I don't really like that behaviour. I would prefer just having $whiteList and $blackList as parameters.

Can you copy the behavior of the new Filter API instead (to preserve consistency): if $properties is null, can be filtered on all filters, else it's a whitelist (I don't like blacklists because they are error-prone, you always miss to update the list when refactoring).

sroze · 2015-05-01T09:56:18Z

Warning1: this is a really painful thing, we shouldn't accept it. @dunglas is there anything in the Hydra specification about it ?

Warning2: in my opinion, for sure properties have to be checked, and errors have to be sent to user !

Moreover, that case of multiple filters is quite useful and may be used a lot.

dunglas · 2015-05-02T12:38:21Z

About warning 1:

The Hydra spec is very flexible and doesn't cover such details: http://www.hydra-cg.com/spec/latest/core/#templated-links

We must take a design decision about filters.

Solution 1

The current one. Everything as root queries like:

/users?myProperty=textToSearch
/users?order[myProperty]=ASC
/users?page=2
/users?itemsPerPage=33 (see Allow getting itemsPerPage from request #44)

Pro:

URL are clean and explicit

Con:

Conflicts can happen and must be handled manually by the developper

Solution 2

Mimic the StrongLoop API:

/users?filter[fields][myProperty]=textToSearch
/users?filter[order][myProperty]=ASC
/users?page=2
/users?itemsPerPage=33

Pro:

No conflict, no manual action must be taken

Con:

URLs looks ugly

As solution 1 is the current implementation (page already conflicts with a filter on a property called page) and solution 2 can always be done manually (with custom filters extending built-in ones), I most favorable to this one. But this can always be changed before the release of 1.0.0. What do you think @sroze @theofidry @amenophis ?

About warning 2:

It was a voluntary choice to ignore to ignore invalid query parameters for reasons similar of those explained here: http://stackoverflow.com/questions/15947074/extra-query-parameters-in-the-rest-api-url
But I've no strong opinion about that. I think it's more confortable and less error prone to ignore invalid parameters (think the tons of client side tools such as Google Analytics that append custom parameters to URLs).

theofidry · 2015-05-02T14:28:02Z

StrongLoop approach is better in my opinion because it is more consistent. You may have different kind of parameters besides filters, and with it you know where to look at without any risk of conflict aside for the filter keyword.

Still, StrongLoop works differently: there API by default have lots of filter for instance including embedded relations or where filters on a property of a related entity. This Bundle favor a more strict API exposed to the client.

The main problem of the current approach is that it uses as much keywords as special filters. As of now there only the order filter and pages so it's not that much of a problem.

So IMO if you deem there will be not much filter such as this one, solution1 is fine. If you fear that they may be more in the future, solution2.

Solution 3: don't choose and add an option to use solution2 instead of solution1 with solution1 as default. I do not have the code right know but I guess It would be just the way of retrieving the filters which would change and that would let the current filters unchanged.

dunglas · 2015-05-02T16:53:10Z

Solution 3 will make maintaining the bundle harder (2 different system to maintain).
Maybe can we slightly change the current API to:

expose URLs similar to StrongLoop by default
allow to easily enable all filters and orders (still disabled by default for performance and security reasons) as there is an interest for that (admin interfaces, non-publics API...)
provide a way to customize URLs parameters exposed by filters (for instance to mimic manually current patterns)

theofidry · 2015-05-15T09:59:05Z

ping @dunglas and @sroze: update of the PR. I have some issues with the Behat tests but it's really weird because they run fine on local but not on Travis.

That being said I Behat tests do not pass anyway. If I do When I send a "GET" request to "/dummies?order[id]=asc" I get wrong @id@, hydra:firstPage and hydra:lastPage values.

Expected response:

{
        "@context": "/contexts/Dummy",
        "@id": "/dummies",
        "@type": "hydra:PagedCollection",
        "hydra:nextPage": "/dummies?page=2",
        "hydra:totalItems": 30,
        "hydra:itemsPerPage": 3,
        "hydra:firstPage": "/dummies",
        "hydra:lastPage": "/dummies?page=10",
        "hydra:member": [...]
}

Actual response:

{
          "@context": "\/contexts\/Dummy",
          "@id": "\/dummies?order[id]=asc",
          "@type": "hydra:PagedCollection",
          "hydra:nextPage": "\/dummies?order%5Bid%5D=asc&page=2",
          "hydra:totalItems": 30,
          "hydra:itemsPerPage": 3,
          "hydra:firstPage": "\/dummies?order%5Bid%5D=asc",
          "hydra:lastPage": "\/dummies?order%5Bid%5D=asc&page=10",
          "hydra:member": [...]
}

dunglas · 2015-05-18T20:15:06Z

The actual response looks OK to me. See #57 (the old behavior was incorrect).

dunglas · 2015-05-18T20:16:37Z

composer.json

@@ -23,7 +23,8 @@
    "doctrine/inflector": "~1.0",
    "doctrine/doctrine-bundle": "~1.2",
    "dunglas/php-property-info": "~0.2",
-    "phpdocumentor/reflection": "^1.0.7"
+    "phpdocumentor/reflection": "^1.0.7",
+    "phpunit/phpunit": "~4.6"


The phpunit dependency should be in the require-dev section.

theofidry · 2015-05-18T20:18:40Z

Okay if you say so, looks ugly though :(

dunglas · 2015-05-18T20:19:20Z

Doctrine/Orm/OrderFilter.php

@@ -0,0 +1,109 @@
+<?php


Can you add the license header please?

dunglas · 2015-05-18T20:32:23Z

The reason is easy to understand: the return of /myurl and /myurl?myfilter=foo is a different set of data. Each set of data must be uniquely identified. Why does it look ugly to you?

dunglas · 2015-05-18T20:33:10Z

Tests/Doctrine/Orm/OrderFilterTest.php

+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Test class for @see Dunglas\ApiBundle\Doctrine\Orm\OrderFilter.


Must be {@see TheClass}.

I actually found the proper annotation: it's @coversDefaultClass and @covers. I will change that tomorrow.

theofidry · 2015-05-18T20:35:38Z

Yes it makes sense, but kind of unexpected because I was used to the old one. It's just personal but I find \/dummies?order%5Bid%5D=asc&page=10 ugly^^, although properly parsed in /dummies?order[id]=asc&page=10 looks a bit better.

dunglas · 2015-05-18T20:36:42Z

Tests/Doctrine/Orm/OrderFilterTest.php

+                ],
+                'SELECT o FROM Dunglas\ApiBundle\Tests\Behat\TestBundle\Entity\Dummy o'
+            ],
+            // Unkown property unabled


typo (enabled?)

What do you mean?

unabled in the comment

dunglas · 2015-05-18T20:39:47Z

Thanks for your hard work @theofidry I hope we can merge this one soon. Just some issues left and some doc needed :)

dunglas · 2015-05-18T20:41:11Z

json_encode encode everything by default but the unencoded version works too!

theofidry · 2015-05-18T20:45:57Z

Is the PHPUnit test good? I was not really happy with it when I did it and felt like I missed few things that should be done here.

dunglas · 2015-05-18T20:49:16Z

IMO using Behat here make more sense and specing the class with PHPSpec instead of PHPUnit would be a better approach must now that you've wrote the test, we can keep it. A test is better than no test :)

theofidry · 2015-05-19T09:53:29Z

Doctrine/Orm/OrderFilter.php

+    {
+        $metadata      = $queryBuilder->getEntityManager()->getClassMetadata($resource->getEntityClass());
+        $fieldNames    = $metadata->getFieldNames();
+        $filterList    = [];


The way I retrieve $fieldNames is incorrect or at least getClassMetadata is quoted as @internal. Wouldn't it be better to use ReflectionClass instead?

Note for myself: remove unused variable.

I suggest injecting the manager instead: https://github.com/dunglas/DunglasApiBundle/blob/master/Doctrine/Orm/Filter.php#L167

Maybe can you add an abstract BaseFilterclass to share logic between Filter and OrderFilter classes?

theofidry · 2015-05-20T10:05:55Z

ping @dunglas could you give it a look again? There is still the getDescription() to do and some problems with the behat the all the others points have been taken cared of.

dunglas · 2015-05-20T19:32:07Z

The main remaining problem IMO is that the API of yout OrderFilter is not coherent with the one of SearchFilter (the second one only has a "all" and "whitelist" mode).

theofidry · 2015-05-21T07:12:35Z

Is that really an issue? Sure at first it may sound inconsistent, but I hardly see any practical use of a blacklist for a search filter since you have to declare the strategy of the enabled properties anyway. On the contrary the blacklist may come handy when you just want to disable only one or two properties. Only keeping the whitelist would require to keep the list of properties async which may be a pain.

theofidry · 2015-05-21T13:13:27Z

@dunglas Ok ready for merge :)

Order filter.

dunglas · 2015-05-21T13:50:09Z

Thank you @theofidry

theofidry mentioned this pull request Apr 30, 2015

Adding order filter #19

Closed

sroze reviewed May 1, 2015
View reviewed changes

This was referenced May 6, 2015

Feature request - Extended date filter #46

Closed

extendable DataProvider #22

Closed

dunglas mentioned this pull request May 6, 2015

Invalid property for filter behavior #42

Closed

theofidry changed the title ~~Added order filter.~~ [WIP] Added order filter. May 13, 2015

theofidry changed the title ~~[WIP] Added order filter.~~ Order filter. May 15, 2015

dunglas reviewed May 18, 2015
View reviewed changes

Doctrine/Orm/OrderFilter.php

@@ -0,0 +1,109 @@

<?php

Copy link

Member

dunglas May 18, 2015

Choose a reason for hiding this comment

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

Can you add the license header please?

dunglas reviewed May 18, 2015
View reviewed changes

dunglas added this to the 1.0.0 milestone May 18, 2015

dunglas added the enhancement label May 18, 2015

theofidry reviewed May 19, 2015
View reviewed changes

Added order filter and AbstractFilter.

6eca171

dunglas added a commit that referenced this pull request May 21, 2015

Merge pull request #43 from theofidry/rebased-order-filter

b53ddf4

Order filter.

dunglas merged commit b53ddf4 into api-platform:master May 21, 2015

theofidry deleted the rebased-order-filter branch June 23, 2015 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order filter. #43

Order filter. #43

theofidry commented Apr 30, 2015

sroze May 1, 2015

dunglas May 18, 2015

sroze commented May 1, 2015

dunglas commented May 2, 2015

theofidry commented May 2, 2015

dunglas commented May 2, 2015

theofidry commented May 15, 2015

dunglas commented May 18, 2015

dunglas May 18, 2015

theofidry commented May 18, 2015

dunglas May 18, 2015

dunglas commented May 18, 2015

dunglas May 18, 2015

theofidry May 18, 2015

theofidry commented May 18, 2015

dunglas May 18, 2015

theofidry May 18, 2015

dunglas May 18, 2015

theofidry May 18, 2015

dunglas commented May 18, 2015

dunglas commented May 18, 2015

theofidry commented May 18, 2015

dunglas commented May 18, 2015

theofidry May 19, 2015

theofidry May 19, 2015

dunglas May 19, 2015

theofidry May 19, 2015

theofidry commented May 20, 2015

dunglas commented May 20, 2015

theofidry commented May 21, 2015

theofidry commented May 21, 2015

dunglas commented May 21, 2015

Order filter. #43

Order filter. #43

Conversation

theofidry commented Apr 30, 2015

Syntax

Warning1

Warning2

Declaration

Blacklisting properties

Code change/remarks

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented May 1, 2015

dunglas commented May 2, 2015

theofidry commented May 2, 2015

dunglas commented May 2, 2015

theofidry commented May 15, 2015

dunglas commented May 18, 2015

Choose a reason for hiding this comment

theofidry commented May 18, 2015

Choose a reason for hiding this comment

dunglas commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented May 18, 2015

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

dunglas commented May 18, 2015

dunglas commented May 18, 2015

theofidry commented May 18, 2015

dunglas commented May 18, 2015

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

theofidry commented May 20, 2015

dunglas commented May 20, 2015

theofidry commented May 21, 2015

theofidry commented May 21, 2015

dunglas commented May 21, 2015