feature: improve ids management for association and fix invalid iri/uuid in SearchFilter #1844

Simperfit · 2018-04-11T10:24:40Z

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

This PRs update the ids management for association in the SearchFilter.

I've not updated the single at the moment because I think it should be done in another PR.
This fixes #775.

Something I'm not sure is the behaviour, do we ignore the filter (this is the current implementation) or do we return 0 elements (like by replacing the invalid values with null instead of logging + return) ?

Simperfit · 2018-04-23T13:43:02Z

@dunglas this needs a review

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

Toflar · 2018-04-30T13:17:55Z

I really hope this can be released as bugfix because right now, the searchfilter is just completely useless if your identifier column is not called id 😢

antograssiot · 2018-04-30T13:23:12Z

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

    {
        parent::__construct($managerRegistry, $requestStack, $logger, $properties);

+        if (null === $identifiersExtractor) {
+            @trigger_error('Not injecting ItemIdentifiersExtractor is deprecated since API Platform 2.1 and will not be possible anymore in API Platform 3.0.', E_USER_DEPRECATED);


2.1 -> 2.3 ?

soyuka · 2018-04-30T13:24:37Z

I really hope this can be released as bugfix because right now, the searchfilter is just completely useless if your identifier column is not called id cry

Only when the field is an identifier and it's not named id 😄 (who would do that seriously ?! /s)

Toflar · 2018-04-30T13:30:15Z

Sure, all my identifiers are uuids and so I named them uuid instead of id but I could change that.

soyuka · 2018-04-30T13:56:30Z

Sure, all my identifiers are uuids and so I named them uuid instead of id but I could change that.

IMHO, you should do your own filter to support uuid on identifiers. I agree it'll be close to a copy/paste of the current SearchFilter but I don't think it's a bad idea. It kinda joins the idea I had above (and could maybe be proposed as a new feature for upcoming releases):

FieldSearchFilter (handles LIKE and such where clauses)

IdentifierSearchFilter (handles strict equalities, IRIs and everything related to identifiers (foo.bar=id). This one would reuse the identifier denormalization introduced a few weeks back).

Simperfit · 2018-05-03T07:56:38Z

I've removed the UUID part.

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

Simperfit · 2018-05-04T09:49:03Z

src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml

@@ -48,7 +48,6 @@
        </service>

        <!-- Filter -->
-


soyuka · 2019-04-06T09:55:51Z

doing this EU-FOSSA Hackathon

antograssiot

LGTM

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

vincentchalamon · 2019-04-06T16:48:27Z

features/doctrine/search_filter.feature

@@ -389,8 +462,7 @@ Feature: Search filter on collections
        "@id": {"pattern": "^/dummies$"},
        "@type": {"pattern": "^hydra:Collection$"},
        "hydra:member": {
-          "type": "array",
-          "maxItems": 0


Please do not remove this check, but replace it with the right value

This is because the test was wrong in the first place, an invalid filter should not be applied and therefore should return every items of the collection. We fixed that here.

vincentchalamon · 2019-04-06T16:51:21Z

src/Bridge/Symfony/Bundle/Resources/config/doctrine_mongodb_odm.xml

@@ -66,7 +66,9 @@
            <argument type="service" id="api_platform.iri_converter" />
            <argument type="service" id="api_platform.property_accessor" />
            <argument type="service" id="logger" on-invalid="ignore" />
+            <argument key="$identifiersExtractor" type="service" id="api_platform.identifiers_extractor.cached" />


Suggested change

<argument key="$identifiersExtractor" type="service" id="api_platform.identifiers_extractor.cached" />

<argument type="service" id="api_platform.identifiers_extractor.cached" on-invalid="ignore" />

vincentchalamon · 2019-04-06T16:51:33Z

src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml

@@ -55,6 +55,7 @@
            <argument type="service" id="api_platform.iri_converter" />
            <argument type="service" id="api_platform.property_accessor" />
            <argument type="service" id="logger" on-invalid="ignore" />
+            <argument key="$identifiersExtractor" type="service" id="api_platform.identifiers_extractor.cached" />


Suggested change

<argument key="$identifiersExtractor" type="service" id="api_platform.identifiers_extractor.cached" />

<argument type="service" id="api_platform.identifiers_extractor.cached" on-invalid="ignore" />

vincentchalamon · 2019-04-06T16:53:32Z

features/doctrine/search_filter.feature

@@ -20,37 +20,62 @@ Feature: Search filter on collections
    When I send a "GET" request to "/dummy_cars?colors.prop=red"
    Then the response status code should be 200
    And the JSON should be deep equal to:
-    """
+		"""


Tab indent instead of spaces

vincentchalamon · 2019-04-06T16:55:15Z

tests/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilterTest.php

@@ -344,15 +345,6 @@ public function provideApplyTestData(): array
                                ],
                            ],
                        ],
-                        [
-                            '$match' => [


Why did you remove this block of test result?

Same reason as above.

vincentchalamon · 2019-04-06T16:55:22Z

tests/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilterTest.php

@@ -469,17 +461,7 @@ public function provideApplyTestData(): array
                    $filterFactory,
                ],
                'invalid value for relation' => [
-                    [
-                        [
-                            '$match' => [


Same reason as above.

meyerbaptiste · 2019-04-06T18:16:25Z

src/Bridge/Doctrine/MongoDbOdm/Filter/SearchFilter.php

    {
        parent::__construct($managerRegistry, $logger, $properties);

+        if (null === $identifiersExtractor) {
+            @trigger_error('Not injecting ItemIdentifiersExtractor is deprecated since API Platform 2.5 and can lead to unexpected behaviors, it will not be possible anymore in API Platform 3.0.', E_USER_DEPRECATED);


Mongo is marked as @experimental. Not sure BC is relevant here...

Simperfit requested review from teohhanhui, soyuka and meyerbaptiste April 11, 2018 10:24

Simperfit force-pushed the feature/improve-ids-management-for-association branch 3 times, most recently from d770734 to 0feb549 Compare April 11, 2018 10:29

Simperfit changed the title ~~Feature/improve ids management for association~~ feature: improve ids management for association and fix invalid iri/uuid in SearchFilter Apr 11, 2018

Simperfit force-pushed the feature/improve-ids-management-for-association branch 4 times, most recently from a9a8112 to 5c3183d Compare April 11, 2018 13:28

Simperfit added [RFR] Ready for Review [WIP] Work In Progress and removed [RFR] Ready for Review labels Apr 11, 2018

Simperfit force-pushed the feature/improve-ids-management-for-association branch from 5c3183d to 1de97f9 Compare April 11, 2018 15:31

Simperfit added [RFR] Ready for Review and removed [WIP] Work In Progress labels Apr 11, 2018

Simperfit force-pushed the feature/improve-ids-management-for-association branch 2 times, most recently from 850aa53 to e6d7809 Compare April 12, 2018 10:02

Toflar mentioned this pull request Apr 18, 2018

Ask target entity for its identifier instead of hardcoding to id #1810

Closed

soyuka reviewed Apr 23, 2018

View reviewed changes

src/Bridge/Doctrine/Orm/Filter/SearchFilter.php Outdated Show resolved Hide resolved

Toflar approved these changes Apr 30, 2018

View reviewed changes

src/Bridge/Doctrine/Orm/Filter/SearchFilter.php Outdated Show resolved Hide resolved

antograssiot reviewed Apr 30, 2018

View reviewed changes

Simperfit force-pushed the feature/improve-ids-management-for-association branch from e6d7809 to b36ab6b Compare May 3, 2018 07:56

meyerbaptiste reviewed May 4, 2018

View reviewed changes

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php Outdated Show resolved Hide resolved

Simperfit commented May 4, 2018

View reviewed changes

Simperfit force-pushed the feature/improve-ids-management-for-association branch 2 times, most recently from f2d5170 to c9a8108 Compare May 4, 2018 13:23

meyerbaptiste force-pushed the feature/improve-ids-management-for-association branch 5 times, most recently from c675488 to bef4e51 Compare May 4, 2018 14:40

meyerbaptiste approved these changes May 4, 2018

View reviewed changes

Simperfit force-pushed the feature/improve-ids-management-for-association branch 2 times, most recently from 11b6582 to f3bb88e Compare May 9, 2018 09:59

Simperfit requested a review from dunglas May 9, 2018 10:58

soyuka added the ⭐ EU-FOSSA Hackathon label Apr 4, 2019

soyuka force-pushed the feature/improve-ids-management-for-association branch 5 times, most recently from 3a7d125 to 34d9e7b Compare April 6, 2019 14:03

antograssiot approved these changes Apr 6, 2019

View reviewed changes

src/Bridge/Doctrine/Orm/Filter/SearchFilter.php Outdated Show resolved Hide resolved

SearchFiler: improve association's id management

6d657b8

soyuka force-pushed the feature/improve-ids-management-for-association branch from 34d9e7b to 6d657b8 Compare April 6, 2019 15:17

soyuka mentioned this pull request Apr 6, 2019

Bug ? Get collection with filters and several IRI #775

Closed

soyuka merged commit 8f57f08 into api-platform:master Apr 6, 2019

vincentchalamon requested changes Apr 6, 2019

View reviewed changes

vincentchalamon mentioned this pull request Apr 6, 2019

Fix typos #2702

Merged

meyerbaptiste reviewed Apr 6, 2019

View reviewed changes

This was referenced Apr 7, 2019

WIP: Use other keys than 'id' for associations in SearchFilter #2388

Closed

Simplify ExistsFilter declaration #2722

Merged

	<argument key="$identifiersExtractor" type="service" id="api_platform.identifiers_extractor.cached" />
	<argument type="service" id="api_platform.identifiers_extractor.cached" on-invalid="ignore" />

Uh oh!

feature: improve ids management for association and fix invalid iri/uuid in SearchFilter #1844

feature: improve ids management for association and fix invalid iri/uuid in SearchFilter #1844

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Simperfit commented Apr 23, 2018

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Toflar commented Apr 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

Simperfit commented May 3, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Apr 6, 2019

Uh oh!

antograssiot left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Simperfit commented Apr 11, 2018 •

edited

Loading

Toflar commented Apr 30, 2018 •

edited

Loading

soyuka commented Apr 30, 2018 •

edited

Loading

soyuka commented Apr 30, 2018 •

edited

Loading