Merge pull request #1800 from soyuka/fix-identifier-cache

soyuka · web-flow · commit b2c6f84f93d0 · 2018-03-30T12:25:58.000+02:00
Fix identifier cache
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;
 
@@ -27,16 +29,21 @@
  */
 class CachedIdentifiersExtractorTest extends TestCase
 {
-    public function identifiersProvider()
+    public function itemProvider()
     {
-        yield [1, 1];
-        yield [$uuid = new Uuid(), $uuid->__toString()];
+        $dummy = new Dummy();
+        $dummy->setId($id = 1);
+        yield [$dummy, ['id' => $id]];
+
+        $dummy = new Dummy();
+        $dummy->setId($id = new Uuid());
+        yield [$dummy, ['id' => $id]];
     }
 
     /**
-     * @dataProvider identifiersProvider
+     * @dataProvider itemProvider
      */
-    public function testFirstPass($identifier, $identifierValue)
+    public function testFirstPass($item, $expected)
     {
         $key = 'iri_identifiers'.md5(Dummy::class);
 
@@ -48,29 +55,25 @@ public function testFirstPass($identifier, $identifierValue)
         $cacheItemPool->getItem($key)->shouldBeCalled()->willReturn($cacheItem);
         $cacheItemPool->save($cacheItem)->shouldBeCalled();
 
-        $dummy = new Dummy();
-        $dummy->setId($identifier);
-
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
-        $decoration->getIdentifiersFromItem($dummy)->shouldBeCalled()->willReturn(['id' => $identifierValue]);
+        $decoration->getIdentifiersFromItem($item)->shouldBeCalled()->willReturn($expected);
 
-        $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));
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item));
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item), 'Trigger the local cache');
 
         $decoration->getIdentifiersFromResourceClass(Dummy::class)->shouldBeCalled()->willReturn(['id']);
 
         $expectedResult = ['id'];
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class));
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class), 'Trigger the local cache');
+        $this->assertSame($expectedResult, $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class));
+        $this->assertSame($expectedResult, $identifiersExtractor->getIdentifiersFromResourceClass(Dummy::class), 'Trigger the local cache');
     }
 
     /**
-     * @dataProvider identifiersProvider
+     * @dataProvider itemProvider
      */
-    public function testSecondPass($identifier, $identifierValue)
+    public function testSecondPass($item, $expected)
     {
         $key = 'iri_identifiers'.md5(Dummy::class);
 
@@ -81,20 +84,49 @@ public function testSecondPass($identifier, $identifierValue)
         $cacheItemPool = $this->prophesize(CacheItemPoolInterface::class);
         $cacheItemPool->getItem($key)->shouldBeCalled()->willReturn($cacheItem);
 
+        $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
+        $decoration->getIdentifiersFromItem($item)->shouldNotBeCalled();
+
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
+
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item));
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item), 'Trigger the local cache');
+    }
+
+    public function identifiersRelatedProvider()
+    {
+        $related = new RelatedDummy();
+        $related->setId($relatedId = 2);
+
         $dummy = new Dummy();
-        $dummy->setId($identifier);
+        $dummy->setId($id = 1);
+        $dummy->setRelatedDummy($related);
 
-        $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
-        $decoration->getIdentifiersFromItem($dummy)->shouldNotBeCalled();
+        yield [$dummy, ['id' => $id, 'relatedDummy' => $relatedId]];
 
-        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null);
+        $related = new RelatedDummy();
+        $related->setId($relatedId = 1);
 
-        $expectedResult = ['id' => $identifierValue];
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');
+        $dummy = new Dummy();
+        $dummy->setId($id = new Uuid());
+        $dummy->setRelatedDummy($related);
+
+        yield [$dummy, ['id' => $id, 'relatedDummy' => $relatedId]];
+
+        $related = new RelatedDummy();
+        $related->setId($relatedId = new Uuid());
+
+        $dummy = new Dummy();
+        $dummy->setId($id = new Uuid());
+        $dummy->setRelatedDummy($related);
+
+        yield [$dummy, ['id' => $id, 'relatedDummy' => $relatedId]];
     }
 
-    public function testFirstPassWithRelated()
+    /**
+     * @dataProvider identifiersRelatedProvider
+     */
+    public function testFirstPassWithRelated($item, $expected)
     {
         $key = 'iri_identifiers'.md5(Dummy::class);
         $keyRelated = 'iri_identifiers'.md5(RelatedDummy::class);
@@ -110,24 +142,19 @@ public function testFirstPassWithRelated()
         $cacheItemPool->getItem($key)->shouldBeCalled()->willReturn($cacheItem);
         $cacheItemPool->getItem($keyRelated)->shouldBeCalled()->willReturn($cacheItemRelated);
 
-        $related = new RelatedDummy();
-        $related->setId(1);
-
-        $dummy = new Dummy();
-        $dummy->setId(1);
-        $dummy->setRelatedDummy($related);
-
         $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
-        $decoration->getIdentifiersFromItem($dummy)->shouldBeCalled()->willReturn(['id' => 1, 'relatedDummy' => 1]);
+        $decoration->getIdentifiersFromItem($item)->shouldBeCalled()->willReturn($expected);
 
-        $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));
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item));
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item), 'Trigger the local cache');
     }
 
-    public function testSecondPassWithRelated()
+    /**
+     * @dataProvider identifiersRelatedProvider
+     */
+    public function testSecondPassWithRelated($item, $expected)
     {
         $key = 'iri_identifiers'.md5(Dummy::class);
         $keyRelated = 'iri_identifiers'.md5(RelatedDummy::class);
@@ -144,20 +171,38 @@ public function testSecondPassWithRelated()
         $cacheItemPool->getItem($key)->shouldBeCalled()->willReturn($cacheItem);
         $cacheItemPool->getItem($keyRelated)->shouldBeCalled()->willReturn($cacheItemRelated);
 
-        $related = new RelatedDummy();
-        $related->setId(1);
+        $decoration = $this->prophesize(IdentifiersExtractorInterface::class);
+        $decoration->getIdentifiersFromItem($item)->shouldNotBeCalled();
 
-        $dummy = new Dummy();
-        $dummy->setId(1);
-        $dummy->setRelatedDummy($related);
+        $identifiersExtractor = new CachedIdentifiersExtractor($cacheItemPool->reveal(), $decoration->reveal(), null, $this->getResourceClassResolver());
+
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item));
+        $this->assertSame($expected, $identifiersExtractor->getIdentifiersFromItem($item), '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);
-        $decoration->getIdentifiersFromItem($dummy)->shouldNotBeCalled();
 
         $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;
+        });
 
-        $expectedResult = ['id' => 1, 'relatedDummy' => 1];
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
-        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');
+        return $resourceClassResolver->reveal();
     }
 }
diff --git a/tests/Api/IdentifiersExtractorTest.php b/tests/Api/IdentifiersExtractorTest.php
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`	`}`