Manage embedded fields for API Platform Filter by jsamouh · Pull Request #984 · api-platform/core

jsamouh · 2017-03-13T08:12:52Z

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

This PR is to aliment a discussion about how to manage embedded fields in API Platform Filter
This PR works form Boolean Filter Only. It works recursively Entity->Embed Fields -> Embed Fields..
No Tests is developped for the moment

soyuka · 2017-03-13T08:28:03Z

    protected function isPropertyMapped(string $property, string $resourceClass, bool $allowAssociation = false): bool
    {
-        if ($this->isPropertyNested($property)) {
+        if ($this->isPropertyNested($property) && !$this->isPropertyNestedEmbedded($property, $resourceClass)) {


Is this condition valid everytime? I mean, will it break current implementations if we put this inside isPropertyNested so that other filters also benefits from this? Maybe it's just the boolean filter that's impacted, but I doubt that.

Yes it will breaks 👍
If we put the code in isPropertyNested, it disturbs for differents reasons:

1/ it means that a embed field is not considered as a nested field. In fact it is by definition :-)

2/ the function extractProperties in AbstractFilter use isPropertyNested and for an Embed field, it has to be needsFixing to true. Otherwise, it fails for the request.

This code is in AbstractFilter, so any Filter will benefit the code.
Normally the only thing to change for each filter is isBooleanField, isDateField, isNumericField...

Okay got it. I'd add a method that ends up doing $this->isPropertyNested($property) && !$this->isPropertyNestedEmbedded($property, $resourceClass) though. Feel free to add tests.

I do not agree that an Embedded field should be considered a "nested property". isNestedProperty is to help us find out when we need to join.

Agreed ofc. Smth like isPropertyNestedFilterable

@teohhanhui , you're right for that. So We confirm that a nested property concerns only join ?

@teohhanhui @soyuka , what about extractProperties method in the class AbstracFilter ? The property $needsFixing to true. Otherwise the embed field embed_b.field is transformed to embed_b_field and generates an error in the request

Yes, that's the approach that I'd take.

Embedded fields are inlined to the field mapping, apparently (see https://github.com/doctrine/doctrine2/blob/v2.5.6/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L3230-L3264), so there should be no further changes necessary.

Yes, we also need to fix extractProperties. $needsFixing = true for embedded fields. 😄

So you may still find it valuable to extract the logic into, say, isPropertyEmbedded.

jsamouh · 2017-03-13T08:35:11Z

To explain a little bit:

Embed field is a nested property. So it makes sense according to me to recognize this field as a nested property. However, we need to dissociate nestedAssociation versus nestedEmbedded.
So I create the method isPropertyNestedEmbedded in AbstractFilter and apply it when necessary. This functions just needs to know if the Metadata owns this fields. An Embed field is managed like a simple field
I modify a little bit the method isBooleanField in BooleanFilter to retrieve metadata in the same way as the function isPropertyMapped in AbstractFilter
I modify the QueryNameGenerator of API platform to replace . by _ cause it generates an error when we set the parameter (not pretty)
I did not develop isPropertyAssociation cause it costs performance so I prefer to use isPropertyNested and use Negation in the code to know if the property is a nested association

soyuka · 2017-03-13T08:41:47Z

Is this issue limited to the booleanFilter?

jsamouh · 2017-03-13T08:44:00Z

@soyuka , for the moment yes. I'm waiting your feedback before to apply it in any filter and write STRONG tests about this

jsamouh · 2017-03-13T15:40:18Z

@teohhanhui @soyuka , second commit for discussion (apply the code in all filters). No unit and functional tests are developped for the moment. But existing tests success

soyuka · 2017-03-13T15:51:55Z

Better, I'd need @teohhanhui to confirm but I think that we break backward changes by adding $resourceClass. This'll have to target master.

@jordscream would be nice to have a functional test involving those embedded properties. For unit tests, coverage looks fine not sure you'd be able to add much.

jsamouh · 2017-03-13T16:48:20Z

@soyuka , no problem for functionnal test, writing in progress

antograssiot · 2017-03-13T17:36:53Z

In unit test, I might be wrong but I wanted to make sure that the filter description worked ok.

jsamouh · 2017-03-14T15:55:43Z

@soyuka @teohhanhui Why we call dropSchema to execute an insert just after ?
https://github.com/api-platform/core/blob/2.0/features/doctrine/date_filter.feature#L267-L269

soyuka · 2017-03-14T16:00:51Z

dropSchema executes after the scenario is complete (https://github.com/api-platform/core/blob/2.0/features/bootstrap/FeatureContext.php#L88).

Doing this there ensures that the next scenario has a clean database

jsamouh · 2017-03-14T16:01:44Z

@soyuka My apologize....

jsamouh · 2017-03-14T17:10:22Z

@soyuka @teohhanhui I had functionnal tests.

Tell me if it is enough. The most important to me is to test of at least on filter a model like this entity.relation.embed.field.

jsamouh · 2017-03-15T16:36:22Z

@soyuka @teohhanhui any news ? :-)

soyuka · 2017-03-15T16:52:31Z

    And the JSON should be equal to:
    """
-    {
+      {


Indentation looks wrong here but it's maybe github failing

soyuka · 2017-03-15T16:54:15Z

+    Then the response status code should be 200
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON should be equal to:


Would it be possible to use a schema here instead? Or you can play with JSON node should equal. It's a pain to maintain when we add more filters.

soyuka · 2017-03-15T16:57:31Z

Same indentation looks weird here from github POV. This is the maintain mess I'm talking about above :p.

soyuka · 2017-03-15T17:00:14Z

Shit failed the review thing \o/. Anyway great job there! However, this has a BC break and should target master.

jsamouh · 2017-03-15T17:24:35Z

@soyuka @teohhanhui , thanks for your feedback. I gonna check indentation and I will rebase to master.

Just 2 questions:

Why it is a BC breaks ?
@soyuka , can you give me more details about your comment ? I am not sure to understand

Thanks API platform Team !

soyuka · 2017-03-15T17:30:08Z

Why it is a BC breaks ?

Because it alters methods that custom filters might use by extending our AbstractFilter.

For example, I use it with extractProperties, if I update it'll break my code because the second argument is mandatory. There was no argument before. Break.

can you give me more details about your comment ? I am not sure to understand

which one?

jsamouh · 2017-03-15T17:31:41Z

@soyuka Ok , thanks for the explanation

This comment:
Would it be possible to use a schema here instead? Or you can play with JSON node should equal. It's a pain to maintain when we add more filters.

soyuka · 2017-03-15T18:05:10Z

Oh yes, just like you did the other tests, you're using And the JSON should be valid according to this schema: instead of And the JSON should be equal to:.

When I speak about nodes you can write tests like this as well:

And the JSON node "_embedded.item" should have 1 element

jsamouh · 2017-03-16T10:56:23Z

@soyuka , not sure of my modification according your comment but I did it in boolean_filter.feature. Can you tell me if it is ok for you before I modify my other functionnal test ?

thanks

soyuka

The first test is correct, the other ones should match a schema instead of comparing the full json. Nice work!

soyuka · 2017-03-16T10:59:36Z

+    Then the response status code should be 200
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON should be equal to:


And the JSON should be valid according to this schema:

soyuka · 2017-03-16T10:59:53Z

+    Then the response status code should be 200
+    And the response should be in JSON
+    And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
+    And the JSON should be equal to:


And the JSON should be valid according to this schema:

jsamouh · 2017-03-16T11:02:55Z

@soyuka
ah, according your comments , you say to me to do the opposite :-)

Oh yes, just like you did the other tests, you're using And the JSON should be valid according to this schema: instead of And the JSON should be equal to

So I need to replace all my the JSON should be equal to by And the JSON should be valid according to this schema ?

soyuka · 2017-03-16T11:09:42Z

Oh sorry about this, rephrasing :

use schema instead of full json equality for the sake of maintainability.

Really sorry that you misunderstood this I was just doing too many things at once yesterday and speaking 3 languages at the same time makes it hard to write comprehensible things sometimes :|.

jsamouh · 2017-04-18T21:26:58Z

@soyuka , I revert my change about functional test to not alter the existing functional test
Let me know if it is okay for you

soyuka · 2017-04-19T06:30:18Z

        parent:    'api_platform.doctrine.orm.order_filter'
-        arguments: [ { 'id': ~, 'name': 'desc', 'relatedDummy.symfony': ~ } ]
-        tags:      [ { name: 'api_platform.filter' } ]
+        arguments: [ { 'id': ~, 'name': 'desc', 'relatedDummy.name': ~, 'embeddedDummy.dummyName': 'desc' } ]


you removed relatedDummy.symfony order, I assume this is wanted? As long as tests pass it's ok to me 👍

soyuka · 2017-04-19T06:31:09Z

php-cs-fixer failed, you can check the diff on travis

jsamouh · 2017-04-21T14:47:01Z

@soyuka done (normally)

meyerbaptiste

Fix my comments and phpstan should be happy!

meyerbaptiste · 2017-04-21T22:40:52Z

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a two `$resourceClass` argument in version API Platform 3.0. Not defining it is deprecated since API Platform 2.1.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);
