Improve #1781 by using ResourceClassResolver

abluchet · abluchet · commit 89276bd1e956 · 2018-03-30T10:44:20.000+02:00
diff --git a/src/Api/CachedIdentifiersExtractor.php b/src/Api/CachedIdentifiersExtractor.php
@@ -33,14 +33,20 @@ final class CachedIdentifiersExtractor implements IdentifiersExtractorInterface
     private $cacheItemPool;
     private $propertyAccessor;
     private $decorated;
+    private $resourceClassResolver;
     private $localCache = [];
     private $localResourceCache = [];
 
-    public function __construct(CacheItemPoolInterface $cacheItemPool, IdentifiersExtractorInterface $decorated, PropertyAccessorInterface $propertyAccessor = null)
+    public function __construct(CacheItemPoolInterface $cacheItemPool, IdentifiersExtractorInterface $decorated, PropertyAccessorInterface $propertyAccessor = null, ResourceClassResolverInterface $resourceClassResolver = null)
     {
         $this->cacheItemPool = $cacheItemPool;
         $this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor();
         $this->decorated = $decorated;
+        $this->resourceClassResolver = $resourceClassResolver;
+
+        if (null === $this->resourceClassResolver) {
+            @trigger_error(sprintf('Not injecting %s in the CachedIdentifiersExtractor might introduce cache issues with object identifiers.', ResourceClassResolverInterface::class), E_USER_DEPRECATED);
+        }
     }
 
     /**
@@ -76,12 +82,12 @@ public function getIdentifiersFromItem($item): array
                 continue;
             }
 
-            if (method_exists($identifiers[$propertyName], '__toString')) {
-                $identifiers[$propertyName] = (string) $identifiers[$propertyName];
+            $relatedResourceClass = $this->getObjectClass($identifiers[$propertyName]);
+
+            if (null !== $this->resourceClassResolver && !$this->resourceClassResolver->isResourceClass($relatedResourceClass)) {
                 continue;
             }
 
-            $relatedResourceClass = $this->getObjectClass($identifiers[$propertyName]);
             if (!$relatedIdentifiers = $this->localCache[$relatedResourceClass] ?? false) {
                 $relatedCacheKey = self::CACHE_KEY_PREFIX.md5($relatedResourceClass);
                 try {
diff --git a/src/Api/IdentifiersExtractor.php b/src/Api/IdentifiersExtractor.php
@@ -32,12 +32,18 @@ final class IdentifiersExtractor implements IdentifiersExtractorInterface
     private $propertyNameCollectionFactory;
     private $propertyMetadataFactory;
     private $propertyAccessor;
+    private $resourceClassResolver;
 
-    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, PropertyAccessorInterface $propertyAccessor = null)
+    public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, PropertyAccessorInterface $propertyAccessor = null, ResourceClassResolverInterface $resourceClassResolver = null)
     {
         $this->propertyNameCollectionFactory = $propertyNameCollectionFactory;
         $this->propertyMetadataFactory = $propertyMetadataFactory;
         $this->propertyAccessor = $propertyAccessor ?? PropertyAccess::createPropertyAccessor();
+        $this->resourceClassResolver = $resourceClassResolver;
+
+        if (null === $this->resourceClassResolver) {
+            @trigger_error(sprintf('Not injecting %s in the CachedIdentifiersExtractor might introduce cache issues with object identifiers.', ResourceClassResolverInterface::class), E_USER_DEPRECATED);
+        }
     }
 
     /**
@@ -68,14 +74,19 @@ public function getIdentifiersFromItem($item): array
             if (null === $identifier || false === $identifier) {
                 continue;
             }
+
             $identifier = $identifiers[$propertyName] = $this->propertyAccessor->getValue($item, $propertyName);
+
             if (!\is_object($identifier)) {
                 continue;
-            } elseif (method_exists($identifier, '__toString')) {
-                $identifiers[$propertyName] = (string) $identifier;
-                continue;
             }
+
             $relatedResourceClass = $this->getObjectClass($identifier);
+
+            if (null !== $this->resourceClassResolver && !$this->resourceClassResolver->isResourceClass($relatedResourceClass)) {
+                continue;
+            }
+
             $relatedItem = $identifier;
             unset($identifiers[$propertyName]);
             foreach ($this->propertyNameCollectionFactory->create($relatedResourceClass) as $relatedPropertyName) {
diff --git a/src/Api/ResourceClassResolver.php b/src/Api/ResourceClassResolver.php
@@ -28,6 +28,7 @@ final class ResourceClassResolver implements ResourceClassResolverInterface
     use ClassInfoTrait;
 
     private $resourceNameCollectionFactory;
+    private $localIsResourceClassCache = [];
 
     public function __construct(ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory)
     {
@@ -69,12 +70,16 @@ 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) {
-                return true;
+                return $this->localIsResourceClassCache[$type] = true;
             }
         }
 
-        return false;
+        return $this->localIsResourceClassCache[$type] = false;
     }
 }
diff --git a/src/Bridge/Symfony/Bundle/Resources/config/api.xml b/src/Bridge/Symfony/Bundle/Resources/config/api.xml
@@ -217,12 +217,14 @@
             <argument type="service" id="api_platform.metadata.property.name_collection_factory" />
             <argument type="service" id="api_platform.metadata.property.metadata_factory" />
             <argument type="service" id="api_platform.property_accessor" />
+            <argument type="service" id="api_platform.resource_class_resolver" />
         </service>
 
         <service id="api_platform.identifiers_extractor.cached" class="ApiPlatform\Core\Api\CachedIdentifiersExtractor" decorates="api_platform.identifiers_extractor" public="false">
             <argument type="service" id="api_platform.cache.identifiers_extractor" />
             <argument type="service" id="api_platform.identifiers_extractor.cached.inner" />
             <argument type="service" id="api_platform.property_accessor" />
+            <argument type="service" id="api_platform.resource_class_resolver" />
         </service>
 
         <!-- Subresources -->
diff --git a/tests/Api/CachedIdentifiersExtractorTest.php b/tests/Api/CachedIdentifiersExtractorTest.php
@@ -15,10 +15,12 @@
 
 use ApiPlatform\Core\Api\CachedIdentifiersExtractor;
 use ApiPlatform\Core\Api\IdentifiersExtractorInterface;
+use ApiPlatform\Core\Api\ResourceClassResolverInterface;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Doctrine\Generator\Uuid;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy;
 use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
 use Psr\Cache\CacheItemInterface;
 use Psr\Cache\CacheItemPoolInterface;
 
@@ -54,7 +56,7 @@ public function testFirstPass($identifier, $identifierValue)
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
         $decoration->getIdentifiersFromItem($dummy)->shouldBeCalled()->willReturn(['id' => $identifierValue]);
 
-        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
 
         $expectedResult = ['id' => $identifierValue];
         $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
@@ -87,7 +89,7 @@ public function testSecondPass($identifier, $identifierValue)
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
         $decoration->getIdentifiersFromItem($dummy)->shouldNotBeCalled();
 
-        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
 
         $expectedResult = ['id' => $identifierValue];
         $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
@@ -120,7 +122,7 @@ public function testFirstPassWithRelated()
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
         $decoration->getIdentifiersFromItem($dummy)->shouldBeCalled()->willReturn(['id' => 1, 'relatedDummy' => 1]);
 
-        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
 
         $expectedResult = ['id' => 1, 'relatedDummy' => 1];
         $this->assertEquals(['id' => 1, 'relatedDummy' => 1], $identifiersExtractor->getIdentifiersFromItem($dummy));
@@ -154,10 +156,36 @@ public function testSecondPassWithRelated()
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
         $decoration->getIdentifiersFromItem($dummy)->shouldNotBeCalled();
 
-        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
 
         $expectedResult = ['id' => 1, 'relatedDummy' => 1];
         $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
         $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');
     }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Not injecting ApiPlatform\Core\Api\ResourceClassResolverInterface in the CachedIdentifiersExtractor might introduce cache issues with object identifiers.
+     */
+    public function testDeprecationResourceClassResolver()
+    {
+        $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class);
+        $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
+
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+    }
+
+    private function getResourceClassResolver()
+    {
+        $resourceClassResolver = $this->prophesize(ResourceClassResolverInterface::class);
+        $resourceClassResolver->isResourceClass(Argument::type('string'))->will(function ($args) {
+            if (Uuid::class === $args[0]) {
+                return false;
+            }
+
+            return true;
+        });
+
+        return $resourceClassResolver->reveal();
+    }
 }
diff --git a/tests/Api/IdentifiersExtractorTest.php b/tests/Api/IdentifiersExtractorTest.php
@@ -14,6 +14,7 @@
 namespace ApiPlatform\Core\Tests\Api;
 
 use ApiPlatform\Core\Api\IdentifiersExtractor;
+use ApiPlatform\Core\Api\ResourceClassResolverInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
 use ApiPlatform\Core\Metadata\Property\PropertyMetadata;
@@ -22,6 +23,7 @@
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy;
 use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\RelatedDummy;
 use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
 
 /**
  * @author Antoine Bluchet <soyuka@gmail.com>
@@ -53,7 +55,7 @@ public function testGetIdentifiersFromResourceClass()
     {
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id']);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $this->assertEquals(['id'], $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class));
     }
@@ -62,7 +64,7 @@ public function testGetCompositeIdentifiersFromResourceClass()
     {
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id', 'name']);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $this->assertEquals(['id', 'name'], $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class));
     }
@@ -71,31 +73,32 @@ public function testGetIdentifiersFromItem()
     {
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id']);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $dummy = new Dummy();
         $dummy->setId(1);
 
         $this->assertEquals(['id' => 1], $identifiersExtractor->getIdentifiersFromItem($dummy));
     }
 
-    public function testGetStringableIdentifiersFromItem()
+    public function testUseObjectIdentifier()
     {
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id']);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
+        $uuid = new Uuid();
         $dummy = new Dummy();
-        $dummy->setId(new Uuid());
+        $dummy->setId($uuid);
 
-        $this->assertEquals(['id' => 'foo'], $identifiersExtractor->getIdentifiersFromItem($dummy));
+        $this->assertEquals(['id' => $uuid], $identifiersExtractor->getIdentifiersFromItem($dummy));
     }
 
     public function testGetCompositeIdentifiersFromItem()
     {
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id', 'name']);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $dummy = new Dummy();
         $dummy->setId(1);
@@ -109,7 +112,7 @@ public function testGetRelatedIdentifiersFromItem()
         $prophecies = $this->getMetadataFactoryProphecies(Dummy::class, ['id', 'relatedDummy']);
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(RelatedDummy::class, ['id'], $prophecies);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $related = new RelatedDummy();
         $related->setId(2);
@@ -130,7 +133,7 @@ public function testThrowNoIdentifierFromItem()
         $prophecies = $this->getMetadataFactoryProphecies(Dummy::class, ['id', 'relatedDummy']);
         list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(RelatedDummy::class, [], $prophecies);
 
-        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal(), null, $this->getResourceClassResolver());
 
         $related = new RelatedDummy();
         $related->setId(2);
@@ -141,4 +144,34 @@ public function testThrowNoIdentifierFromItem()
 
         $identifiersExtractor->getIdentifiersFromItem($dummy);
     }
+
+    private function getResourceClassResolver()
+    {
+        $resourceClassResolver = $this->prophesize(ResourceClassResolverInterface::class);
+        $resourceClassResolver->isResourceClass(Argument::type('string'))->will(function ($args) {
+            if (Uuid::class === $args[0]) {
+                return false;
+            }
+
+            return true;
+        });
+
+        return $resourceClassResolver->reveal();
+    }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Not injecting ApiPlatform\Core\Api\ResourceClassResolverInterface in the CachedIdentifiersExtractor might introduce cache issues with object identifiers.
+     */
+    public function testLegacyGetIdentifiersFromItem()
+    {
+        list($propertyNameCollectionFactoryProphecy, $propertyMetadataFactoryProphecy) = $this->getMetadataFactoryProphecies(Dummy::class, ['id']);
+
+        $identifiersExtractor = new IdentifiersExtractor($propertyNameCollectionFactoryProphecy->reveal(), $propertyMetadataFactoryProphecy->reveal());
+
+        $dummy = new Dummy();
+        $dummy->setId(1);
+
+        $this->assertEquals(['id' => 1], $identifiersExtractor->getIdentifiersFromItem($dummy));
+    }
 }
diff --git a/tests/Bridge/Symfony/Routing/IriConverterTest.php b/tests/Bridge/Symfony/Routing/IriConverterTest.php

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ final class ResourceClassResolver implements ResourceClassResolverInterface`
`28`	`28`	`use ClassInfoTrait;`
`29`	`29`
`30`	`30`	`private $resourceNameCollectionFactory;`
	`31`	`+ private $localIsResourceClassCache = [];`
`31`	`32`
`32`	`33`	`public function __construct(ResourceNameCollectionFactoryInterface $resourceNameCollectionFactory)`
`33`	`34`	`{`
`@@ -69,12 +70,16 @@ public function getResourceClass($value, string $resourceClass = null, bool $str`
`69`	`70`	`*/`
`70`	`71`	`public function isResourceClass(string $type): bool`
`71`	`72`	`{`
	`73`	`+ if (isset($this->localIsResourceClassCache[$type])) {`
	`74`	`+ return $this->localIsResourceClassCache[$type];`
	`75`	`+ }`
	`76`	`+`
`72`	`77`	`foreach ($this->resourceNameCollectionFactory->create() as $resourceClass) {`
`73`	`78`	`if ($type === $resourceClass) {`
`74`		`- return true;`
	`79`	`+ return $this->localIsResourceClassCache[$type] = true;`
`75`	`80`	`}`
`76`	`81`	`}`
`77`	`82`
`78`		`- return false;`
	`83`	`+ return $this->localIsResourceClassCache[$type] = false;`
`79`	`84`	`}`
`80`	`85`	`}`