Skip to content

Commit

Permalink
Rework how resource class resolver use inheritance
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Nek-
Nek- committed Apr 7, 2019
1 parent a5c1919 commit a9b5d09
Show file tree
Hide file tree
Showing 13 changed files with 260 additions and 39 deletions.
21 changes: 21 additions & 0 deletions features/bootstrap/DoctrineContext.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyOffer;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyProduct;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyProperty;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyTableInheritanceNotApiResourceChild;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyTableInheritanceNotApiResourceChild as DummyTableInheritanceNotApiResourceChildDocument;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\EmbeddableDummy;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\EmbeddedDummy;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\FileConfigDummy;
Expand Down Expand Up @@ -165,6 +167,17 @@ public function thereAreDummyObjects(int $nb)
$this->manager->flush();
}

/**
* @When some dummy table inheritance data but not api resource child are created
*/
public function someDummyTableInheritanceDataButNotApiResourceChildAreCreated()
{
$dummy = $this->buildDummyTableInheritanceNotApiResourceChild();
$dummy->setName('Foobarbaz inheritance');
$this->manager->persist($dummy);
$this->manager->flush();
}

/**
* @Given there are :nb foo objects with fake names
*/
Expand Down Expand Up @@ -1272,6 +1285,14 @@ private function buildDummy()
return $this->isOrm() ? new Dummy() : new DummyDocument();
}

/**
* @return DummyTableInheritanceNotApiResourceChild|DummyTableInheritanceNotApiResourceChildDocument
*/
private function buildDummyTableInheritanceNotApiResourceChild()
{
return $this->isOrm() ? new DummyTableInheritanceNotApiResourceChild() : new DummyTableInheritanceNotApiResourceChildDocument();
}

/**
* @return DummyAggregateOffer|DummyAggregateOfferDocument
*/
Expand Down
85 changes: 68 additions & 17 deletions features/main/table_inheritance.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceChild$"
"pattern": "^DummyTableInheritanceChild$",
"required": "true"
},
"@context": {
"type": "string",
"pattern": "^/contexts/DummyTableInheritanceChild$"
"pattern": "^/contexts/DummyTableInheritanceChild$",
"required": "true"
},
"@id": {
"type": "string",
"pattern": "^/dummy_table_inheritance_children/1$"
"pattern": "^/dummy_table_inheritance_children/1$",
"required": "true"
},
"name": {
"type": "string",
Expand Down Expand Up @@ -61,7 +64,8 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceChild$"
"pattern": "^DummyTableInheritanceChild$",
"required": "true"
},
"name": {
"type": "string",
Expand All @@ -81,6 +85,40 @@ Feature: Table inheritance
"""

@createSchema
Scenario: Some children not api resources are created in the app
When some dummy table inheritance data but not api resource child are created
And I send a "GET" request to "/dummy_table_inheritances"
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 valid according to this schema:
"""
{
"type": "object",
"properties": {
"hydra:member": {
"type": "array",
"items": {
"type": "object",
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritance(Child)?$",
"required": "true"
},
"name": {
"type": "string",
"required": "true"
}
}
},
"minItems": 1
}
},
"required": ["hydra:member"]
}
"""

Scenario: Create a table inherited resource
When I add "Content-Type" header equal to "application/ld+json"
And I send a "POST" request to "/dummy_table_inheritance_children" with body:
Expand All @@ -97,15 +135,18 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceChild$"
"pattern": "^DummyTableInheritanceChild$",
"required": "true"
},
"@context": {
"type": "string",
"pattern": "^/contexts/DummyTableInheritanceChild$"
"pattern": "^/contexts/DummyTableInheritanceChild$",
"required": "true"
},
"@id": {
"type": "string",
"pattern": "^/dummy_table_inheritance_children/1$"
"pattern": "^/dummy_table_inheritance_children/1$",
"required": "true"
},
"name": {
"type": "string",
Expand Down Expand Up @@ -136,15 +177,18 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceDifferentChild$"
"pattern": "^DummyTableInheritanceDifferentChild$",
"required": "true"
},
"@context": {
"type": "string",
"pattern": "^/contexts/DummyTableInheritanceDifferentChild$"
"pattern": "^/contexts/DummyTableInheritanceDifferentChild$",
"required": "true"
},
"@id": {
"type": "string",
"pattern": "^/dummy_table_inheritance_different_children/2$"
"pattern": "^/dummy_table_inheritance_different_children/2$",
"required": "true"
},
"name": {
"type": "string",
Expand Down Expand Up @@ -181,15 +225,18 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceRelated$"
"pattern": "^DummyTableInheritanceRelated$",
"required": "true"
},
"@context": {
"type": "string",
"pattern": "^/contexts/DummyTableInheritanceRelated$"
"pattern": "^/contexts/DummyTableInheritanceRelated$",
"required": "true"
},
"@id": {
"type": "string",
"pattern": "^/dummy_table_inheritance_relateds/1$"
"pattern": "^/dummy_table_inheritance_relateds/1$",
"required": "true"
},
"children": {
"items": {
Expand All @@ -199,7 +246,8 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceChild$"
"pattern": "^DummyTableInheritanceChild$",
"required": "true"
},
"name": {
"type": "string",
Expand All @@ -215,7 +263,8 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceDifferentChild$"
"pattern": "^DummyTableInheritanceDifferentChild$",
"required": "true"
},
"name": {
"type": "string",
Expand Down Expand Up @@ -255,7 +304,8 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceChild$"
"pattern": "^DummyTableInheritanceChild$",
"required": "true"
},
"name": {
"type": "string",
Expand All @@ -271,7 +321,8 @@ Feature: Table inheritance
"properties": {
"@type": {
"type": "string",
"pattern": "^DummyTableInheritanceDifferentChild$"
"pattern": "^DummyTableInheritanceDifferentChild$",
"required": "true"
},
"name": {
"type": "string",
Expand Down
6 changes: 4 additions & 2 deletions src/Api/IdentifiersExtractor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,10 @@ public function getIdentifiersFromResourceClass(string $resourceClass): array
public function getIdentifiersFromItem($item): array
{
$identifiers = [];
$type = $this->getObjectClass($item);
$resourceClass = $this->resourceClassResolver->isResourceClass($type) ? $this->resourceClassResolver->getResourceClass($item) : $type;
$resourceClass = $this->getObjectClass($item);
if (null !== $this->resourceClassResolver) { // BC Layer
$resourceClass = $this->resourceClassResolver->isResourceClass($resourceClass) ? $this->resourceClassResolver->getResourceClass($item) : $resourceClass;
}

foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $propertyName) {
$propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $propertyName);
Expand Down
36 changes: 27 additions & 9 deletions src/Api/ResourceClassResolver.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ final class ResourceClassResolver implements ResourceClassResolverInterface
use ClassInfoTrait;

private $resourceNameCollectionFactory;
private $localIsResourceClassCache = [];

public function __construct(ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory)
{
Expand Down Expand Up @@ -56,20 +55,27 @@ public function getResourceClass($value, string $resourceClass = null, bool $str
$resolvedResourceClass = $currentResourceClass;
}
}

if (null !== $resolvedResourceClass) {
$resourceClass = $resolvedResourceClass;
}

$typeIsStrictResourceClass = $this->isStrictResourceClass($type);
if ($strict && $typeIsStrictResourceClass) {
return $type;
}

$typeIsResourceClass = $this->isResourceClass($type);
if (
null === $type
|| ((!$strict || $resourceClass === $type) && $isResourceClass = $this->isResourceClass($type))
|| null !== $resolvedResourceClass && interface_exists($resourceClass)
|| (((!$strict || $resourceClass === $type)) && $typeIsResourceClass)
|| null !== $resolvedResourceClass && (interface_exists($resourceClass) || !$typeIsStrictResourceClass)
) {
return $resourceClass;
}

if (
($isResourceClass ?? $this->isResourceClass($type))
$typeIsResourceClass
|| (is_subclass_of($type, $resourceClass) && $this->isResourceClass($resourceClass))
) {
return $type;
Expand All @@ -83,16 +89,28 @@ public function getResourceClass($value, string $resourceClass = null, bool $str
*/
public function isResourceClass(string $type): bool
{
if (isset($this->localIsResourceClassCache[$type])) {
return $this->localIsResourceClassCache[$type];
foreach ($this->resourceNameCollectionFactory->create() as $resourceClass) {
if ($type === $resourceClass || is_subclass_of($type, $resourceClass)) {
return true;
}
}

return false;
}

/**
* Same of isResourceClass but more strict: it ignores inheritance.
*
* @param string $type FQCN of an object
*/
private function isStrictResourceClass(string $type): bool
{
foreach ($this->resourceNameCollectionFactory->create() as $resourceClass) {
if ($type === $resourceClass || is_subclass_of($type, $resourceClass)) {
return $this->localIsResourceClassCache[$type] = true;
if ($type === $resourceClass) {
return true;
}
}

return $this->localIsResourceClassCache[$type] = false;
return false;
}
}
2 changes: 1 addition & 1 deletion src/Api/ResourceClassResolverInterface.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ interface ResourceClassResolverInterface
/**
* Guesses the associated resource.
*
* @param object $value Object you're playing with
* @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
*
Expand Down
5 changes: 4 additions & 1 deletion src/Bridge/Doctrine/EventListener/PublishMercureUpdatesListener.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ private function reset(): void
private function storeEntityToPublish($entity, string $property): void
{
$resourceClass = $this->getObjectClass($entity);
if (!$this->resourceClassResolver->isResourceClass($resourceClass)) {

if ($this->resourceClassResolver->isResourceClass($resourceClass)) {
$resourceClass = $this->resourceClassResolver->getResourceClass($entity);
} else {
return;
}

Expand Down
1 change: 1 addition & 0 deletions tests/Api/IdentifiersExtractorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ private function getResourceClassResolver()
$resourceClassResolver->isResourceClass(Argument::type('string'))->will(function ($args) {
return !(Uuid::class === $args[0]);
});
$resourceClassResolver->getResourceClass(Argument::any())->will(function ($args) {return \get_class($args[0]); });

return $resourceClassResolver->reveal();
}
Expand Down
28 changes: 25 additions & 3 deletions tests/Api/ResourceClassResolverTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyTableInheritance;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyTableInheritanceChild;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyTableInheritanceNotApiResourceChild;
use PHPUnit\Framework\TestCase;

/**
Expand Down Expand Up @@ -175,13 +176,12 @@ public function testGetResourceClassWithChildResource()

public function testGetResourceClassWithInterfaceResource()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage("The given object's resource is the interface \"ApiPlatform\Core\Tests\Fixtures\DummyResourceInterface\", finding a class is not possible.");
$dummy = new DummyResourceImplementation();
$resourceNameCollectionFactoryProphecy = $this->prophesize(ResourceNameCollectionFactoryInterface::class);
$resourceNameCollectionFactoryProphecy->create()->willReturn(new ResourceNameCollection([DummyResourceInterface::class]))->shouldBeCalled();

$resourceClassResolver = new ResourceClassResolver($resourceNameCollectionFactoryProphecy->reveal());
$resourceClassResolver->getResourceClass($dummy, DummyResourceInterface::class, true);
$this->assertEquals(DummyResourceInterface::class, $resourceClassResolver->getResourceClass($dummy, DummyResourceInterface::class, true));
}

public function testGetResourceClassWithoutSecondParameterIsAccurate()
Expand Down Expand Up @@ -212,4 +212,26 @@ public function testInterfaceCanBeResourceClass()
$resourceClassResolver = new ResourceClassResolver($resourceNameCollectionFactoryProphecy->reveal());
$this->assertTrue($resourceClassResolver->isResourceClass(DummyResourceImplementation::class));
}

public function testItResolveParentResourceClassOfChildrenOfResourceClasses()
{
$dummy = new DummyTableInheritanceNotApiResourceChild();
$resourceNameCollectionFactoryProphecy = $this->prophesize(ResourceNameCollectionFactoryInterface::class);
$resourceNameCollectionFactoryProphecy->create()->willReturn(new ResourceNameCollection([DummyTableInheritance::class]))->shouldBeCalled();

$resourceClassResolver = new ResourceClassResolver($resourceNameCollectionFactoryProphecy->reveal());
// $this->assertTrue($resourceClassResolver->isResourceClass(DummyTableInheritanceNotApiResourceChild::class));
// $this->assertEquals(DummyTableInheritance::class, $resourceClassResolver->getResourceClass($dummy));
$this->assertEquals(DummyTableInheritance::class, $resourceClassResolver->getResourceClass($dummy, DummyTableInheritance::class, true));
}

public function testItResolveChildWhenParentAndChildrenAreResourcesAndIsStrict()
{
$dummy = new DummyTableInheritanceChild();
$resourceNameCollectionFactoryProphecy = $this->prophesize(ResourceNameCollectionFactoryInterface::class);
$resourceNameCollectionFactoryProphecy->create()->willReturn(new ResourceNameCollection([DummyTableInheritance::class, DummyTableInheritanceChild::class]))->shouldBeCalled();

$resourceClassResolver = new ResourceClassResolver($resourceNameCollectionFactoryProphecy->reveal());
$this->assertEquals(DummyTableInheritanceChild::class, $resourceClassResolver->getResourceClass($dummy, DummyTableInheritance::class, true));
}
}
Loading

0 comments on commit a9b5d09

Please sign in to comment.