Add ExistsFilter #906

teohhanhui · 2017-01-12T06:27:18Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes (thanks @soyuka!)
Fixed tickets	N/A
License	MIT
Doc PR	TODO

IS NULL / IS NOT NULL (field / single-valued association)
or
IS EMPTY / IS NOT EMPTY (collection-valued association)

vincentchalamon · 2017-01-18T12:58:11Z

Please add unit & functional tests

soyuka · 2017-02-10T16:28:42Z

@teohhanhui I let you do the rebase thing okay?

teohhanhui · 2017-02-13T03:34:33Z

@vincentchalamon @soyuka I think we should allow the query parameter key ("exists") to be changed, right? Similar to "order".

soyuka · 2017-02-13T08:55:01Z

@teohhanhui I'm not sure that's needed but I can add it. Let me know.

teohhanhui · 2017-02-13T08:56:16Z

It's harmless even if there's a boolean property named "exists", which is also nullable. We've already handled it safely.

But we can test that in a Behat scenario. 😆

soyuka · 2017-02-13T09:00:56Z

Can you be more specific I didn't get that 😆

Should I add a behat test for exists?

teohhanhui · 2017-02-13T08:59:34Z

tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php

+     * Provides test data.
+     *
+     * Provides 4 parameters:
+     *  - order parameter name


Outdated phpdoc.

teohhanhui · 2017-02-13T09:00:45Z

tests/Fixtures/TestBundle/Entity/Dummy.php

@@ -22,7 +22,7 @@
 *
 * @author Kévin Dunglas <dunglas@gmail.com>
 *
- * @ApiResource(attributes={"filters"={"my_dummy.search", "my_dummy.order", "my_dummy.date", "my_dummy.range", "my_dummy.boolean", "my_dummy.numeric"}})
+ * @ApiResource(attributes={"filters"={"my_dummy.search", "my_dummy.order", "my_dummy.date", "my_dummy.range", "my_dummy.boolean", "my_dummy.numeric", "my_dummy.exists"}})


Can't we split this nicely into one line per filter (and sorted alphabetically)?

teohhanhui · 2017-02-13T09:05:29Z

We do have Behat tests for all the filters, e.g. https://github.com/api-platform/core/blob/master/features/doctrine/order_filter.feature

What I meant is, we can add a boolean "exists" property in the dummy resource, and enable the BooleanFilter for that property. It should not impact the functionality of the ExistsFilter, because it uses a different structure from the BooleanFilter for the query parameters.

soyuka · 2017-02-13T09:07:06Z

Okay adding that.

vincentchalamon · 2017-02-13T09:27:39Z

@teohhanhui 👍 for changing exists key. I also recommend a Behat test to check does not exist feature. /cc @soyuka

soyuka · 2017-02-13T09:36:33Z

@vincentchalamon doesn't a non-conflict test suffice? I understand the need to change the "order" key but for "exists" it seems overdone.

soyuka · 2017-02-13T09:38:21Z

did the modifications but I think travis has an issue when I do the push on your repository @teohhanhui

teohhanhui · 2017-02-13T09:38:30Z

I'd actually love to see order_parameter_name deprecated if we can find out / make sure that there's no conflict.

soyuka · 2017-02-13T09:38:58Z

@teohhanhui Sometimes you just want to name that "sort" though \o/

vincentchalamon · 2017-02-13T09:39:09Z

@soyuka 👍
But I still recommend a does not exist test 😉

soyuka · 2017-02-13T09:40:54Z

I've done that: https://github.com/api-platform/core/pull/906/files#diff-a6db6250a11c29a00faf3d4ae19a769fR10

teohhanhui · 2017-02-13T09:41:44Z

features/doctrine/exists_filter.feature

+  Scenario: delete
+
+  @createSchema
+  Scenario: Get collection that does not exists


Grammar: does not exists

teohhanhui · 2017-02-13T09:42:31Z

One moment. Let me rebase...

teohhanhui · 2017-02-13T09:46:52Z

Rebased.

vincentchalamon · 2017-02-13T09:43:47Z

features/doctrine/exists_filter.feature

+          "maxItems": 0
+        },
+        "hydra:view": {
+          "@id": {"pattern": "^/dummies\\?exists=[exists=0]"},


I think it should be "^/dummies\\?exists[exists]=0", isn't it?

vincentchalamon · 2017-02-13T09:45:23Z

features/doctrine/exists_filter.feature

+          "minItems": 3
+        },
+        "hydra:view": {
+          "@id": {"pattern": "^/dummies\\?exists=[exists=1]"},


Should be "^/dummies\\?exists[exists]=1" I guess

vincentchalamon · 2017-02-13T09:47:02Z

tests/Fixtures/TestBundle/Entity/Dummy.php

+    /**
+     * @var bool
+     *
+     * @ORM\Column(type="boolean", nullable=true, name="existsd")


Typo on name? existsd => exists

EXISTS is an SQL keyword. But we should probably use a name that doesn't look like a typo.

it_exists? 🤣

is_exist should be more consistent, no?

@teohhanhui you do or I do the changes?

@soyuka Do you think we should get another PR merged into 2.0 branch to make some of the other filters more strict in terms of which properties they can be applied to? I'd argue that it should not be considered a BC break, for example if the RangeFilter stopped working for boolean properties (lol) or if the SearchFilter stopped working with a numeric property (really really bad idea in the first place).

Hmm, something that would prevent from doing a NumericFilter on boolean for example?

I think that would be great indeed, with nice error messages to guide the developer.

teohhanhui · 2017-02-13T10:21:06Z

The test failures indicate some of the other filters are not as strict as they should be...

soyuka · 2017-02-13T10:33:17Z

It's just because I added that exists property no?

teohhanhui · 2017-02-13T10:57:51Z

It's just because I added that exists property no?

Yeah, and because of that we have to add the property in the tests for some other filters where $properties is null 😞

soyuka · 2017-02-13T11:15:13Z

Yeah anyway I've already fixed the tests, pushing in a few minutes

soyuka · 2017-02-13T11:19:59Z

Though here we could not add a SearchFilter on exists. What can we do about that?

dunglas · 2017-03-03T16:50:30Z

I would prefer to get rid of Behat completely and write a nice API testing package. But it will require to rewrite our whole test suite.

soyuka · 2017-03-03T19:27:33Z

I would prefer to get rid of Behat completely and write a nice API testing package. But it will require to rewrite our whole test suite.

OMG I'm so +1 on this... I started writing some api tests with nodejs (request+superagent - nice chaining ideas there), but it's lacking a few things to write clean and easy-to-write tests (ie mainly json-based feature management).

I'm all in if you need help on writing the tests again, and if you want to brainstorm on how we get to test APIs better!

antograssiot · 2017-03-05T08:18:53Z

features/doctrine/exists_filter.feature

+          "maxItems": 0
+        },
+        "hydra:view": {
+          "@id": {"pattern": "^/dummies\\?dummyBoolean[exists]=0"},


This json schema is not correctly constructed. You could put anything in the pattern, tests would still pass.
See #967
(Same goes bellow)

soyuka · 2017-03-13T08:30:29Z

@teohhanhui lmk if this is good to you we can merge it.

teohhanhui · 2017-03-13T08:52:18Z

@soyuka I think we still need another approval. 🤣

soyuka · 2017-03-13T08:53:17Z

As you wish 😛

Simperfit · 2017-03-16T11:54:11Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

+
+        $field = $propertyParts['field'];
+
+        if ($metadata->hasAssociation($field)) {


Can't we refacto that to avoid that complexity ?

You mean to reduce the cyclomatic complexity?

Look at my commit (made my changes in a separate commit), I refactored it a bit.

Simperfit

Just a little comment, but LGTM

soyuka · 2017-03-16T13:36:52Z

I'm now officially a git rebase master \o/ @Simperfit done what I could (removed 3 lines of code 😆 ).

teohhanhui · 2017-03-20T06:55:34Z

Thanks @soyuka! ^^

kingschnulli · 2017-05-23T10:17:56Z

Just tested this, but it does not read the assocation properties when autowiring - is there any reason for this? Works fine when defining them by using the configuration

soyuka · 2017-05-23T12:17:22Z

@aggrosoft I'm not sure how autowiring should work with variable constructors but you can check how I make it work in the PR that adds Filter annotations.

kingschnulli · 2017-05-24T07:58:23Z

Shouldn't something like this make the autowiring work:

kingschnulli@d29384b

Add ExistsFilter

teohhanhui · 2018-12-19T19:45:02Z

@aggrosoft I think you've found a (very old, probably introduced in #242 😱) bug!

Sorry for not noticing earlier. 🙈 🤣

(See the diff around here: https://github.com/api-platform/core/pull/242/files#diff-023fcb894b9b3ad337dee44f2ba0a53aR242 - coincidentally also line 242 🍀)

teohhanhui · 2018-12-19T19:48:08Z

~~Hmm... But I think at that time getFieldNames must have included the association names too, otherwise I wouldn't have written the code for that?!~~

Never mind. I don't find anything like that in older versions of Doctrine's code. So I think it's just been broken all this while...

teohhanhui requested review from vincentchalamon and theofidry January 18, 2017 12:41

teohhanhui commented Feb 13, 2017

View reviewed changes

soyuka force-pushed the exists-filter branch from bd738a7 to e0c1dcb Compare February 13, 2017 09:37

soyuka force-pushed the exists-filter branch from e0c1dcb to c6f1768 Compare February 13, 2017 09:39

teohhanhui commented Feb 13, 2017

View reviewed changes

teohhanhui force-pushed the exists-filter branch from c6f1768 to fc2343e Compare February 13, 2017 09:46

vincentchalamon requested changes Feb 13, 2017

View reviewed changes

antograssiot reviewed Mar 5, 2017

View reviewed changes

soyuka force-pushed the exists-filter branch from e25a40f to 42bcbc4 Compare March 6, 2017 08:14

soyuka mentioned this pull request Mar 8, 2017

hydra:view schema validation (#967) #970

Closed

soyuka force-pushed the exists-filter branch 4 times, most recently from 910a951 to 3967d1f Compare March 12, 2017 11:13

Simperfit reviewed Mar 16, 2017

View reviewed changes

Simperfit approved these changes Mar 16, 2017

View reviewed changes

soyuka force-pushed the exists-filter branch 2 times, most recently from 5f8a34f to a3f47ad Compare March 16, 2017 13:30

teohhanhui and others added 2 commits March 16, 2017 14:35

Add ExistsFilter

ae2b7c0

Tests exists filter

3a1e9c2

soyuka force-pushed the exists-filter branch from a3f47ad to 3a1e9c2 Compare March 16, 2017 13:35

teohhanhui merged commit 8053c55 into api-platform:master Mar 20, 2017

teohhanhui deleted the exists-filter branch March 20, 2017 06:55

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#906 from teohhanhui/exists-filter

5c16f73

Add ExistsFilter

teohhanhui mentioned this pull request Mar 8, 2019

Improve the ExistsFilter behavior (to cohabit with the SearchFilter) #2243

Merged

4 tasks


		$field = $propertyParts['field'];

		if ($metadata->hasAssociation($field)) {

Uh oh!

Add ExistsFilter #906

Add ExistsFilter #906

Uh oh!

Conversation

teohhanhui commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentchalamon commented Jan 18, 2017

Uh oh!

soyuka commented Feb 10, 2017

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

teohhanhui commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

vincentchalamon commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

vincentchalamon commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

teohhanhui commented Feb 13, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

teohhanhui commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

soyuka commented Feb 13, 2017

Uh oh!

dunglas commented Mar 3, 2017

Uh oh!

soyuka commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented Jan 12, 2017 •

edited

Loading

teohhanhui commented Feb 13, 2017 •

edited

Loading

soyuka commented Mar 3, 2017 •

edited

Loading

soyuka Mar 16, 2017 •

edited

Loading

teohhanhui commented Dec 19, 2018 •

edited

Loading