fix (IriConverter): localOperationCache is always written but never read

src/Laravel/Routing/IriConverter.php

@@ -140,11 +140,16 @@ public function getIriFromResource(object|string $resource, int $referenceType =
             !$operation->getName()
             || ($operation instanceof HttpOperation && 'POST' === $operation->getMethod())
         ) {
-            $forceCollection = $operation instanceof CollectionOperationInterface;
-            try {
-                $operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation(null, $forceCollection, true);
-                $identifiersExtractorOperation = $operation;
-            } catch (OperationNotFoundException) {
+            if (isset($this->localOperationCache[$localOperationCacheKey])) {
+                $operation = $this->localOperationCache[$localOperationCacheKey];
+                $identifiersExtractorOperation = $this->localIdentifiersExtractorOperationCache[$localOperationCacheKey] ?? $operation;
+            } else {
+                $forceCollection = $operation instanceof CollectionOperationInterface;
+                try {
+                    $operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation(null, $forceCollection, true);
+                    $identifiersExtractorOperation = $operation;
+                } catch (OperationNotFoundException) {
+                }
             }
         }
 

src/Laravel/Tests/Unit/Routing/IriConverterTest.php

@@ -87,4 +87,113 @@ public function testLocalCacheKeyDistinguishesItemAndCollectionForStringResource
             ['uri_variables' => ['id' => 1]],
         ));
     }
+
+    public function testGetIriFromResourceWithoutOperationUsesTheLocalOperationCache(): void
+    {
+        $itemOpName = 'item_op';
+        $itemOp = (new Get())->withName($itemOpName)->withClass(Book::class);
+
+        $router = $this->createMock(RouterInterface::class);
+        $router->expects($this->exactly(2))
+            ->method('generate')
+            ->willReturn('/api/books/1');
+
+        $resourceMetadataFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataFactory->expects($this->once())
+            ->method('create')
+            ->with(Book::class)
+            ->willReturn(new ResourceMetadataCollection(Book::class, [
+                (new ApiResource())->withOperations(new Operations([$itemOpName => $itemOp])),
+            ]));
+
+        $iriConverter = $this->createIriConverter($router, $resourceMetadataFactory);
+
+        // No operation argument: the path every relation and every collection item's @id takes.
+        $context = ['uri_variables' => ['id' => 1]];
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+    }
+
+    public function testGetIriFromResourceWithoutOperationReusesTheCachedOperation(): void
+    {
+        $cachedOp = (new Get())->withName('cached_op')->withClass(Book::class);
+        $staleOp = (new Get())->withName('stale_op')->withClass(Book::class);
+
+        $router = $this->createMock(RouterInterface::class);
+        $router->method('generate')
+            ->willReturnCallback(static fn (string $routeName): string => 'cached_op' === $routeName ? '/api/books/1' : '/api/stale/1');
+
+        // Hand back a different operation on a hypothetical second call: if the cache is not
+        // read, the second IRI visibly changes.
+        $resourceMetadataFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataFactory->method('create')->willReturnOnConsecutiveCalls(
+            new ResourceMetadataCollection(Book::class, [(new ApiResource())->withOperations(new Operations(['cached_op' => $cachedOp]))]),
+            new ResourceMetadataCollection(Book::class, [(new ApiResource())->withOperations(new Operations(['stale_op' => $staleOp]))]),
+        );
+
+        $iriConverter = $this->createIriConverter($router, $resourceMetadataFactory);
+
+        $context = ['uri_variables' => ['id' => 1]];
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+    }
+
+    public function testLocalOperationCacheDistinguishesItemAndCollectionAcrossCacheReads(): void
+    {
+        $collectionOpName = 'collection_op';
+        $itemOpName = 'item_op';
+
+        $collectionOp = (new GetCollection())->withName($collectionOpName)->withClass(Book::class);
+        $itemOp = (new Get())->withName($itemOpName)->withClass(Book::class);
+
+        $router = $this->createMock(RouterInterface::class);
+        $router->expects($this->exactly(4))
+            ->method('generate')
+            ->willReturnCallback(function (string $routeName) use ($collectionOpName, $itemOpName): string {
+                if ($collectionOpName === $routeName) {
+                    return '/api/books';
+                }
+                if ($itemOpName === $routeName) {
+                    return '/api/books/1';
+                }
+                $this->fail(\sprintf('Unexpected route name "%s".', $routeName));
+            });
+
+        // getOperation(null, $forceCollection, true) picks between the two according to $forceCollection.
+        $resourceMetadataFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataFactory->expects($this->exactly(2))
+            ->method('create')
+            ->with(Book::class)
+            ->willReturn(new ResourceMetadataCollection(Book::class, [
+                (new ApiResource())->withOperations(new Operations([
+                    $collectionOpName => $collectionOp,
+                    $itemOpName => $itemOp,
+                ])),
+            ]));
+
+        $iriConverter = $this->createIriConverter($router, $resourceMetadataFactory);
+
+        // Both forms use a string resource, so only the item/collection part of the key differs.
+        // Interleaved on purpose: the third and fourth calls must read what the first two wrote.
+        $context = ['uri_variables' => ['id' => 1]];
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+        $this->assertSame('/api/books', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, new GetCollection()));
+        $this->assertSame('/api/books/1', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, null, $context));
+        $this->assertSame('/api/books', $iriConverter->getIriFromResource(Book::class, UrlGeneratorInterface::ABS_PATH, new GetCollection()));
+    }
+
+    private function createIriConverter(RouterInterface $router, ResourceMetadataCollectionFactoryInterface $resourceMetadataFactory): IriConverter
+    {
+        $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class);
+        $resourceClassResolver->method('isResourceClass')->willReturn(true);
+
+        return new IriConverter(
+            $this->createStub(ProviderInterface::class),
+            $this->createStub(OperationMetadataFactoryInterface::class),
+            $router,
+            $this->createStub(IdentifiersExtractorInterface::class),
+            $resourceClassResolver,
+            $resourceMetadataFactory,
+        );
+    }
 }

src/Symfony/Routing/IriConverter.php

@@ -161,11 +161,16 @@ public function getIriFromResource(object|string $resource, int $referenceType =
             !$operation->getName()
             || ($operation instanceof HttpOperation && 'POST' === $operation->getMethod())
         ) {
-            $forceCollection = $operation instanceof CollectionOperationInterface;
-            try {
-                $operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation(null, $forceCollection, true);
-                $identifiersExtractorOperation = $operation;
-            } catch (OperationNotFoundException) {
+            if (isset($this->localOperationCache[$localOperationCacheKey])) {
+                $operation = $this->localOperationCache[$localOperationCacheKey];
+                $identifiersExtractorOperation = $this->localIdentifiersExtractorOperationCache[$localOperationCacheKey] ?? $operation;
+            } else {
+                $forceCollection = $operation instanceof CollectionOperationInterface;
+                try {
+                    $operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation(null, $forceCollection, true);
+                    $identifiersExtractorOperation = $operation;
+                } catch (OperationNotFoundException) {
+                }
             }
         }
 

tests/Symfony/Routing/IriConverterTest.php

@@ -110,6 +110,92 @@ public function testGetIriFromItemWithContextOperation(): void
         $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item, UrlGeneratorInterface::ABS_URL, $operation));
     }
 
+    public function testGetIriFromItemWithoutOperationUsesTheLocalOperationCache(): void
+    {
+        $item = new Dummy();
+        $item->setId(1);
+
+        $operationName = 'operation_name';
+        $operation = (new Get())->withName($operationName);
+
+        $routerProphecy = $this->prophesize(RouterInterface::class);
+        $routerProphecy->generate($operationName, ['id' => 1], UrlGeneratorInterface::ABS_PATH)->shouldBeCalledTimes(2)->willReturn('/dummies/1');
+
+        $identifiersExtractorProphecy = $this->prophesize(IdentifiersExtractorInterface::class);
+        $identifiersExtractorProphecy->getIdentifiersFromItem($item, $operation, Argument::any())->shouldBeCalledTimes(2)->willReturn(['id' => 1]);
+
+        $resourceMetadataCollectionFactoryProphecy = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataCollectionFactoryProphecy->create(Dummy::class)->shouldBeCalledOnce()->willReturn(new ResourceMetadataCollection(Dummy::class, [
+            (new ApiResource())->withOperations(new Operations([$operationName => $operation])),
+        ]));
+
+        $iriConverter = $this->getIriConverter(null, $routerProphecy, $identifiersExtractorProphecy, $resourceMetadataCollectionFactoryProphecy);
+
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+    }
+
+    public function testGetIriFromItemWithoutOperationReusesTheCachedOperation(): void
+    {
+        $item = new Dummy();
+        $item->setId(1);
+
+        $cachedOperation = (new Get())->withName('cached_operation');
+        $staleOperation = (new Get())->withName('stale_operation');
+
+        $routerProphecy = $this->prophesize(RouterInterface::class);
+        $routerProphecy->generate('cached_operation', ['id' => 1], UrlGeneratorInterface::ABS_PATH)->willReturn('/dummies/1');
+        $routerProphecy->generate('stale_operation', ['id' => 1], UrlGeneratorInterface::ABS_PATH)->willReturn('/stale/1');
+
+        $identifiersExtractorProphecy = $this->prophesize(IdentifiersExtractorInterface::class);
+        $identifiersExtractorProphecy->getIdentifiersFromItem($item, Argument::cetera())->willReturn(['id' => 1]);
+
+        // Prophecy returns these in order across consecutive calls, repeating the last.
+        $resourceMetadataCollectionFactoryProphecy = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataCollectionFactoryProphecy->create(Dummy::class)->willReturn(
+            new ResourceMetadataCollection(Dummy::class, [(new ApiResource())->withOperations(new Operations(['cached_operation' => $cachedOperation]))]),
+            new ResourceMetadataCollection(Dummy::class, [(new ApiResource())->withOperations(new Operations(['stale_operation' => $staleOperation]))]),
+        );
+
+        $iriConverter = $this->getIriConverter(null, $routerProphecy, $identifiersExtractorProphecy, $resourceMetadataCollectionFactoryProphecy);
+
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+    }
+
+    public function testLocalOperationCacheDistinguishesItemAndCollectionIris(): void
+    {
+        $item = new Dummy();
+        $item->setId(1);
+
+        $itemOperation = (new Get())->withName('item_operation')->withClass(Dummy::class);
+        $collectionOperation = (new GetCollection())->withName('collection_operation')->withClass(Dummy::class);
+
+        $routerProphecy = $this->prophesize(RouterInterface::class);
+        $routerProphecy->generate('item_operation', ['id' => 1], UrlGeneratorInterface::ABS_PATH)->shouldBeCalledTimes(2)->willReturn('/dummies/1');
+        $routerProphecy->generate('collection_operation', [], UrlGeneratorInterface::ABS_PATH)->shouldBeCalledTimes(2)->willReturn('/dummies');
+
+        $identifiersExtractorProphecy = $this->prophesize(IdentifiersExtractorInterface::class);
+        $identifiersExtractorProphecy->getIdentifiersFromItem($item, Argument::cetera())->willReturn(['id' => 1]);
+
+        // getOperation(null, $forceCollection, true) picks between the two according to $forceCollection.
+        $resourceMetadataCollectionFactoryProphecy = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class);
+        $resourceMetadataCollectionFactoryProphecy->create(Dummy::class)->shouldBeCalledTimes(2)->willReturn(new ResourceMetadataCollection(Dummy::class, [
+            (new ApiResource())->withOperations(new Operations([
+                'item_operation' => $itemOperation,
+                'collection_operation' => $collectionOperation,
+            ])),
+        ]));
+
+        $iriConverter = $this->getIriConverter(null, $routerProphecy, $identifiersExtractorProphecy, $resourceMetadataCollectionFactoryProphecy);
+
+        // Interleaved on purpose: the third and fourth calls must read the cache entry the first two wrote.
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+        $this->assertSame('/dummies', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, new GetCollection()));
+        $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item));
+        $this->assertSame('/dummies', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, new GetCollection()));
+    }
+
     public function testGetIriFromItemWithNoOperations(): void
     {
         $this->expectExceptionMessage(\sprintf('Unable to generate an IRI for the item of type "%s"', Dummy::class));