Resource resolver for inheritance #2714

Nek- · 2019-04-07T13:32:06Z

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

The resource class resolver now works well with inheritance and is able to resolve resources using the ApiPlatform resource list (more clever work, was needed considering isResource is doing it!)
This change adds a new feature: inherited items from API resources now are working as an API, a new behat test proves it
some parts of ApiPlatform were not compatible because it does not uses the resource resolver but "get class" to get the resource, theses parts are also fixed

Notice: the cache from the resource class resolver is removed here. It was introduced in 2015 and was probably a great improvement, but since 2016 resources are not re-generated on each call to the factory but we call instead of a cache factory. This cache was just adding complexity so I removed it.

Before this patch, when you configure an interface as resource the serializer was not understanding the implementation was a resource. It also fix the behavior of child of ApiResources that were not detected as it.

soyuka

Nice job, few comments and we need to fix mongodb also.

soyuka · 2019-04-08T14:14:13Z

tests/Fixtures/TestBundle/Entity/DummyTableInheritanceNotApiResourceChild.php

+/**
+ * @ORM\Entity
+ */
+class DummyTableInheritanceNotApiResourceChild extends DummyTableInheritance


NonApiResourceTableInheritedDummyChild would be better? @teohhanhui do you have a name in mind? :p

Well, starting by "DummyTableInheritance" seems to have been a convention for similar objects.

tests/Fixtures/TestBundle/Entity/DummyTableInheritanceNotApiResourceChild.php

soyuka · 2019-04-08T14:18:18Z

src/Bridge/Symfony/Routing/IriConverter.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getIriFromItemWithResource($item, string $resourceClass, int $referenceType = UrlGeneratorInterface::ABS_PATH): string


WDYT about getIriFromItemWithContext? Where we'd add a $context['resource_class']? If we have other use cases in the future at least we won't have to change this interface again.

I don't see any use case soon. I think it's confusing considering the main use case is to pass it from another context (serialization context). This seems good to me.

Also, I hate config-arrays. If you want it I'm in favor of creating a config object.

Why not just keep the same interface but add an optional argument that's commented out?

Why not, especially that it doesn't really make sense to not add the resourceClass to the getIriFromItem. We didn't really changed many interfaces though, isn't this a bit cleaner?

src/Bridge/Symfony/Routing/IriConverter.php

src/Bridge/Doctrine/EventListener/PublishMercureUpdatesListener.php

soyuka · 2019-04-08T14:26:26Z

src/Api/ResourceClassResolverInterface.php

+     * @param mixed  $value         Object you're playing with
+     * @param string $resourceClass Resource class it is supposed to be (could be parent class for instance)
+     * @param bool   $strict        value must be type of resource class given or it will return type
+     *


For next reviewers I'm to keep these comment for future developers because it's hard to guess.

The comments need to be rewritten as they don't make much sense now.

src/Api/ResourceClassResolver.php

alanpoulain · 2019-04-08T16:36:17Z

features/main/table_inheritance.feature

+            "properties": {
+              "@type": {
+                "type": "string",
+                "pattern": "^DummyTableInheritance(Child)?$",


We're not really testing there is at least one child, right?

Most of the tests are not actually testing the data considering they are verifying "any" of the given definitions. Scenarios are dependants. see #1724

I don't understand why the dependency between scenarios is an issue here.
You're right about the first part though. But maybe we can avoid this for this scenario?

I don't understand why the dependency between scenarios is an issue here.

It's always an issue when you want to run 1 failing test case without running the whole suite. tests should respect the FIRST principle.

Sure, I agree with this. But I don't see the link with what I said.

I think there are both types. But we should make sure DummyTableInheritanceChild is here at least once.

can't wait to use #2608 would much more easier to deal with...

@alanpoulain oh, it's possible to be sure the item is here at least once? How?

you might be able use tuple validation maybe ? (that's only 3 items... https://json-schema.org/understanding-json-schema/reference/array.html#tuple-validation)

We will use multiple "contains": https://json-schema.org/latest/json-schema-validation.html#rfc.section.6.4.6

Nek- · 2019-04-16T15:52:51Z

Appveyor is a liar. I'm pretty sure this is also cool on Windows! è_é

features/main/table_inheritance.feature

antograssiot · 2019-04-29T15:10:47Z

features/main/table_inheritance.feature

+            "properties": {
+              "@type": {
+                "type": "string",
+                "pattern": "^DummyTableInheritance(Child)?$",


you might be able use tuple validation maybe ? (that's only 3 items... https://json-schema.org/understanding-json-schema/reference/array.html#tuple-validation)

antograssiot · 2019-04-29T15:15:00Z

features/main/table_inheritance.feature

+                "type": "string",
+                "required": "true"
+              }
+            }


Suggested change

}

},

"additionalProperties": false

and the same can be added in other tests to be more accurate

features/main/table_inheritance.feature

tests/Serializer/AbstractItemNormalizerTest.php

tests/Api/ResourceClassResolverTest.php

src/Api/ResourceClassResolver.php

src/Api/ResourceIriConverterInterface.php

teohhanhui · 2019-04-30T16:57:20Z

About the Behat test suite, I could only say most of the tests that use JSON Schema are wrong. I started fixing some of them in #2723 but I'm afraid it's been abandoned.

This commits comes with many changes: 1. The resource class resolver now works well with inheritance and is able to resolve resources using the ApiPlatform resource list (more clever) 2. This change adds a new feature: inherited items from api resources now are working as an api, a new behat test proves it 3. some parts of api platform were not compatible because it does not uses the resource resolver but "get class" to get the resource, theses parts are also fixed Notice: the cache from the resource class resolver is removed here. It was introduced in 2015 and was probably a great improvement, but since 2016 resources are not re-genarated on each call to the factory but we call instead a cached factory. This cache was just adding complexity so I removed it.

@vincentchalamon

Previous commits fix the behavior with non api resources inherited from resource but relation management was still not ok since the update was not done on the abstract item normalizer. The resource class resolver is now used as it should in every normalizers. Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760

soyuka

I think we're almost there with this, let's keep the new interface for now.

teohhanhui · 2019-05-03T15:16:07Z

Okay, so I think this is not ideal. Why don't we inject ResourceClassResolver into IriConverter? That way we can keep the old interface, and the caller does not need to care about which resource class to pass into IriConverter, as indeed that should not be their job. The IriConverter should know how to do its job.

teohhanhui · 2019-05-03T15:19:30Z

See for example https://github.com/api-platform/core/blob/v2.4.2/src/EventListener/WriteListener.php#L80

Are we supposed to do the resource class resolving there? It makes no sense to me. As a caller, I have a resource item, and I want to get the IRI for it. That's all.

soyuka · 2019-05-05T09:05:35Z

Sounds like a good alternative, wdyt @Nek- ?

teohhanhui · 2019-05-06T07:55:10Z

But perhaps the IriConverter would need the attributes, if it needs to do something different based on the resource_class?

dunglas · 2019-04-08T16:23:11Z

src/Api/IriConverterInterface.php

@@ -21,6 +21,8 @@
 * Converts item and resources to IRI and vice versa.
 *
 * @author Kévin Dunglas <dunglas@gmail.com>
+ *
+ * @deprecated in favor of ResourceIriConverterInterface to be removed in ApiPlatform 3.0


I'm against deprecating this interface. It's the heart of API Platform.

I think your concern is legit. We could add a new parameter to the existing method. It would lose a bit of its sense. But really I think you're right about that fact.

Nek- · 2019-05-06T18:39:38Z

@teohhanhui I will give it a try as soon as I have the time for it. I don't really think it will work considering it will have a huge impact on the whole app. (actually, I was thinking about not deprecating the existing method).

teohhanhui · 2019-05-15T20:38:04Z

I'm working on an alternative approach at #2797

Nek- · 2019-05-16T21:01:28Z

@teohhanhui feel free to take tests

teohhanhui · 2019-05-16T21:04:49Z

@Nek- Yes, I'll take the Behat tests. 😄

teohhanhui · 2019-05-17T19:03:48Z

Replaced by #2797

Thanks @Nek- for your work! 😄

Nek- changed the base branch from master to 2.4 April 7, 2019 13:32

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

Fix resource resolution for interface as resources

a5c1919

Before this patch, when you configure an interface as resource the serializer was not understanding the implementation was a resource. It also fix the behavior of child of ApiResources that were not detected as it.

Nek- force-pushed the feature/resource-resolver-for-inheritance branch 5 times, most recently from a9b5d09 to acc9fda Compare April 7, 2019 18:24

soyuka requested changes Apr 8, 2019

View reviewed changes

alanpoulain reviewed Apr 8, 2019

View reviewed changes

src/Api/ResourceClassResolver.php Outdated Show resolved Hide resolved

alanpoulain reviewed Apr 8, 2019

View reviewed changes

Nek- force-pushed the feature/resource-resolver-for-inheritance branch 5 times, most recently from 52a7bdf to f1bcbe4 Compare April 15, 2019 14:51

Nek- mentioned this pull request Apr 24, 2019

"No item route associated with the type" exception using class inheritance #2759

Closed

Nek- force-pushed the feature/resource-resolver-for-inheritance branch 2 times, most recently from 6dcaee0 to 4956250 Compare April 27, 2019 17:55

vincentchalamon mentioned this pull request Apr 29, 2019

Get resource from parent #2760

Closed

4 tasks

Nek- force-pushed the feature/resource-resolver-for-inheritance branch from 4956250 to af70ddf Compare April 29, 2019 09:43

antograssiot reviewed Apr 29, 2019

View reviewed changes

teohhanhui reviewed Apr 30, 2019

View reviewed changes

src/Api/ResourceClassResolver.php Outdated Show resolved Hide resolved

teohhanhui reviewed Apr 30, 2019

View reviewed changes

src/Api/ResourceIriConverterInterface.php Outdated Show resolved Hide resolved

teohhanhui mentioned this pull request Apr 30, 2019

Migrate "/me" routes to API Platform dunglas/ShopApiPlugin#14

Merged

Nek- force-pushed the feature/resource-resolver-for-inheritance branch 3 times, most recently from 2f0db8e to 674975f Compare May 2, 2019 14:52

Nek- added 2 commits May 2, 2019 16:54

Nek- force-pushed the feature/resource-resolver-for-inheritance branch from 674975f to 96d0556 Compare May 2, 2019 14:54

soyuka approved these changes May 3, 2019

View reviewed changes

dunglas requested changes May 6, 2019

View reviewed changes

teohhanhui mentioned this pull request May 15, 2019

Fix ResourceClassResolver handling of inheritance #2797

Merged

1 task

teohhanhui closed this May 17, 2019

Resource resolver for inheritance #2714

Resource resolver for inheritance #2714

Conversation

Nek- commented Apr 7, 2019 • edited Loading

soyuka left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka May 3, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoulain Apr 9, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nek- commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui commented Apr 30, 2019

soyuka left a comment

Choose a reason for hiding this comment

teohhanhui commented May 3, 2019 • edited Loading

teohhanhui commented May 3, 2019 • edited Loading

soyuka commented May 5, 2019

teohhanhui commented May 6, 2019 • edited Loading

dunglas Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nek- commented May 6, 2019

teohhanhui commented May 15, 2019

Nek- commented May 16, 2019

teohhanhui commented May 16, 2019

teohhanhui commented May 17, 2019

Nek- commented Apr 7, 2019 •

edited

Loading

soyuka May 3, 2019 •

edited

Loading

alanpoulain Apr 9, 2019 •

edited

Loading

teohhanhui commented May 3, 2019 •

edited

Loading

teohhanhui commented May 3, 2019 •

edited

Loading

teohhanhui commented May 6, 2019 •

edited

Loading

dunglas Apr 8, 2019 •

edited

Loading