From 12eeb46a26f03dcf5b088a4c137507009a078a61 Mon Sep 17 00:00:00 2001 From: Johannes Werbrouck Date: Sat, 22 Aug 2026 09:56:28 +0200 Subject: [PATCH 1/4] fix(serializer): localOperationCache is always written but never read --- src/Symfony/Routing/IriConverter.php | 15 ++-- tests/Symfony/Routing/IriConverterTest.php | 86 ++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Routing/IriConverter.php b/src/Symfony/Routing/IriConverter.php index eb5ce19576f..00a353081a2 100644 --- a/src/Symfony/Routing/IriConverter.php +++ b/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) { + } } } diff --git a/tests/Symfony/Routing/IriConverterTest.php b/tests/Symfony/Routing/IriConverterTest.php index 0e05743a303..b4be06edf49 100644 --- a/tests/Symfony/Routing/IriConverterTest.php +++ b/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)); From f3f91f76e3aa83c424b43040d8f18df19d004018 Mon Sep 17 00:00:00 2001 From: Johannes Werbrouck Date: Sat, 22 Aug 2026 09:56:28 +0200 Subject: [PATCH 2/4] fix(serializer): localOperationCache is always written but never read --- src/Laravel/Routing/IriConverter.php | 15 ++- .../Tests/Unit/Routing/IriConverterTest.php | 109 ++++++++++++++++++ src/Symfony/Routing/IriConverter.php | 15 ++- tests/Symfony/Routing/IriConverterTest.php | 86 ++++++++++++++ 4 files changed, 215 insertions(+), 10 deletions(-) diff --git a/src/Laravel/Routing/IriConverter.php b/src/Laravel/Routing/IriConverter.php index 82afad6be8a..1e2f56ea9d0 100644 --- a/src/Laravel/Routing/IriConverter.php +++ b/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) { + } } } diff --git a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php b/src/Laravel/Tests/Unit/Routing/IriConverterTest.php index c88de228bda..83bfc744087 100644 --- a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php +++ b/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(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, + ); + } } diff --git a/src/Symfony/Routing/IriConverter.php b/src/Symfony/Routing/IriConverter.php index eb5ce19576f..00a353081a2 100644 --- a/src/Symfony/Routing/IriConverter.php +++ b/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) { + } } } diff --git a/tests/Symfony/Routing/IriConverterTest.php b/tests/Symfony/Routing/IriConverterTest.php index 0e05743a303..b4be06edf49 100644 --- a/tests/Symfony/Routing/IriConverterTest.php +++ b/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)); From 22f2dcb375213aed7fe82662bf21c812ca56370a Mon Sep 17 00:00:00 2001 From: Johannes Werbrouck Date: Mon, 24 Aug 2026 08:41:46 +0200 Subject: [PATCH 3/4] fix(serializer): localOperationCache is always written but never read --- src/Laravel/Tests/Unit/Routing/IriConverterTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php b/src/Laravel/Tests/Unit/Routing/IriConverterTest.php index 83bfc744087..16c28394656 100644 --- a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php +++ b/src/Laravel/Tests/Unit/Routing/IriConverterTest.php @@ -121,7 +121,7 @@ public function testGetIriFromResourceWithoutOperationReusesTheCachedOperation() $router = $this->createMock(RouterInterface::class); $router->method('generate') - ->willReturnCallback(fn (string $routeName): string => 'cached_op' === $routeName ? '/api/books/1' : '/api/stale/1'); + ->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. From 3035f498d9ba297e5a9301efe8f67f57ba5c39b5 Mon Sep 17 00:00:00 2001 From: soyuka Date: Tue, 1 Sep 2026 16:29:26 +0200 Subject: [PATCH 4/4] fix(symfony): item_uri_template in iri cache key Reading the local operation cache on the no-operation path exposed a key collision: an item_uri_template that resolves to no operation leaves $operation null, so the key matches a plain call, while the skolem check and the resource class promotion are both skipped. The first caller then wins for the rest of the request. Also drops the redundant "reuses the cached operation" tests and rewrites the new Symfony ones with PHPUnit mocks instead of Prophecy. --- .../Tests/Unit/Routing/IriConverterTest.php | 28 +--- src/Symfony/Routing/IriConverter.php | 4 +- tests/Symfony/Routing/IriConverterTest.php | 158 +++++++++++------- 3 files changed, 104 insertions(+), 86 deletions(-) diff --git a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php b/src/Laravel/Tests/Unit/Routing/IriConverterTest.php index 16c28394656..fe9643afbe8 100644 --- a/src/Laravel/Tests/Unit/Routing/IriConverterTest.php +++ b/src/Laravel/Tests/Unit/Routing/IriConverterTest.php @@ -96,6 +96,7 @@ public function testGetIriFromResourceWithoutOperationUsesTheLocalOperationCache $router = $this->createMock(RouterInterface::class); $router->expects($this->exactly(2)) ->method('generate') + ->with($itemOpName, ['id' => 1], UrlGeneratorInterface::ABS_PATH) ->willReturn('/api/books/1'); $resourceMetadataFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class); @@ -108,31 +109,6 @@ public function testGetIriFromResourceWithoutOperationUsesTheLocalOperationCache $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)); @@ -159,7 +135,6 @@ public function testLocalOperationCacheDistinguishesItemAndCollectionAcrossCache $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') @@ -174,7 +149,6 @@ public function testLocalOperationCacheDistinguishesItemAndCollectionAcrossCache $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())); diff --git a/src/Symfony/Routing/IriConverter.php b/src/Symfony/Routing/IriConverter.php index 00a353081a2..ea2ee88ffc8 100644 --- a/src/Symfony/Routing/IriConverter.php +++ b/src/Symfony/Routing/IriConverter.php @@ -129,7 +129,9 @@ public function getIriFromResource(object|string $resource, int $referenceType = $operation = $this->operationMetadataFactory->create($context['item_uri_template']); } - $localOperationCacheKey = ($operation?->getName() ?? '').$resourceClass.(\is_string($resource) ? '_s' : '_o').($operation instanceof CollectionOperationInterface ? '_c' : '_i'); + // item_uri_template steers both the skolem check and the resource class promotion below, + // so it has to be part of the key even when it did not resolve to an operation. + $localOperationCacheKey = ($operation?->getName() ?? '').$resourceClass.(\is_string($resource) ? '_s' : '_o').($operation instanceof CollectionOperationInterface ? '_c' : '_i').($context['item_uri_template'] ?? ''); if ($operation && isset($this->localOperationCache[$localOperationCacheKey])) { return $this->generateSymfonyRoute($resource, $referenceType, $this->localOperationCache[$localOperationCacheKey], $context, $this->localIdentifiersExtractorOperationCache[$localOperationCacheKey] ?? null); } diff --git a/tests/Symfony/Routing/IriConverterTest.php b/tests/Symfony/Routing/IriConverterTest.php index b4be06edf49..f987a11d72d 100644 --- a/tests/Symfony/Routing/IriConverterTest.php +++ b/tests/Symfony/Routing/IriConverterTest.php @@ -118,82 +118,106 @@ public function testGetIriFromItemWithoutOperationUsesTheLocalOperationCache(): $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'); + $router = $this->createMock(RouterInterface::class); + $router->expects($this->exactly(2)) + ->method('generate') + ->with($operationName, ['id' => 1], UrlGeneratorInterface::ABS_PATH) + ->willReturn('/dummies/1'); - $identifiersExtractorProphecy = $this->prophesize(IdentifiersExtractorInterface::class); - $identifiersExtractorProphecy->getIdentifiersFromItem($item, $operation, Argument::any())->shouldBeCalledTimes(2)->willReturn(['id' => 1]); + $identifiersExtractor = $this->createStub(IdentifiersExtractorInterface::class); + $identifiersExtractor->method('getIdentifiersFromItem')->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])), - ])); + $resourceMetadataCollectionFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class); + $resourceMetadataCollectionFactory->expects($this->once()) + ->method('create') + ->with(Dummy::class) + ->willReturn(new ResourceMetadataCollection(Dummy::class, [ + (new ApiResource())->withOperations(new Operations([$operationName => $operation])), + ])); - $iriConverter = $this->getIriConverter(null, $routerProphecy, $identifiersExtractorProphecy, $resourceMetadataCollectionFactoryProphecy); + $iriConverter = $this->getMockedIriConverter($router, $resourceMetadataCollectionFactory, $identifiersExtractor); $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item)); $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item)); } - public function testGetIriFromItemWithoutOperationReusesTheCachedOperation(): void + public function testLocalOperationCacheDistinguishesItemAndCollectionIris(): 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)); + $itemOperationName = 'item_operation'; + $collectionOperationName = 'collection_operation'; + + $router = $this->createMock(RouterInterface::class); + $router->expects($this->exactly(4)) + ->method('generate') + ->willReturnCallback(fn (string $routeName): string => match ($routeName) { + $itemOperationName => '/dummies/1', + $collectionOperationName => '/dummies', + default => $this->fail(\sprintf('Unexpected route name "%s".', $routeName)), + }); + + $resourceMetadataCollectionFactory = $this->createMock(ResourceMetadataCollectionFactoryInterface::class); + $resourceMetadataCollectionFactory->expects($this->exactly(2)) + ->method('create') + ->with(Dummy::class) + ->willReturn(new ResourceMetadataCollection(Dummy::class, [ + (new ApiResource())->withOperations(new Operations([ + $itemOperationName => (new Get())->withName($itemOperationName)->withClass(Dummy::class), + $collectionOperationName => (new GetCollection())->withName($collectionOperationName)->withClass(Dummy::class), + ])), + ])); + + $iriConverter = $this->getMockedIriConverter($router, $resourceMetadataCollectionFactory); + + // Both forms pass a string resource, so only the item/collection part of the key differs. + $context = ['uri_variables' => ['id' => 1]]; + $this->assertSame('/dummies/1', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, null, $context)); + $this->assertSame('/dummies', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, new GetCollection())); + $this->assertSame('/dummies/1', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, null, $context)); + $this->assertSame('/dummies', $iriConverter->getIriFromResource(Dummy::class, UrlGeneratorInterface::ABS_PATH, new GetCollection())); } - public function testLocalOperationCacheDistinguishesItemAndCollectionIris(): void + public function testLocalOperationCacheKeyAccountsForItemUriTemplate(): 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]); + $dummyOperation = (new Get())->withName('dummy_operation')->withClass(Dummy::class); + $relatedOperation = (new Get())->withName('related_operation')->withClass(RelatedDummy::class); + + $router = $this->createStub(RouterInterface::class); + $router->method('generate')->willReturnCallback(fn (string $routeName): string => match ($routeName) { + 'dummy_operation' => '/dummies/1', + 'related_operation' => '/related_dummies/1', + default => $this->fail(\sprintf('Unexpected route name "%s".', $routeName)), + }); + + $identifiersExtractor = $this->createStub(IdentifiersExtractorInterface::class); + $identifiersExtractor->method('getIdentifiersFromItem')->willReturn(['id' => 1]); + + // An item_uri_template makes the converter skip the resource class promotion, so the same + // object resolves against Dummy instead of the promoted RelatedDummy. + $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class); + $resourceClassResolver->method('isResourceClass')->willReturn(true); + $resourceClassResolver->method('getResourceClass')->willReturn(RelatedDummy::class); + + $resourceMetadataCollectionFactory = $this->createStub(ResourceMetadataCollectionFactoryInterface::class); + $resourceMetadataCollectionFactory->method('create')->willReturnCallback( + static fn (string $resourceClass): ResourceMetadataCollection => new ResourceMetadataCollection($resourceClass, [ + (new ApiResource())->withOperations(new Operations( + RelatedDummy::class === $resourceClass + ? ['related_operation' => $relatedOperation] + : ['dummy_operation' => $dummyOperation] + )), + ]) + ); - // 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, - ])), - ])); + $operationMetadataFactory = $this->createStub(OperationMetadataFactoryInterface::class); + $operationMetadataFactory->method('create')->willReturn(null); - $iriConverter = $this->getIriConverter(null, $routerProphecy, $identifiersExtractorProphecy, $resourceMetadataCollectionFactoryProphecy); + $iriConverter = $this->getMockedIriConverter($router, $resourceMetadataCollectionFactory, $identifiersExtractor, $resourceClassResolver, $operationMetadataFactory); - // 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())); + $this->assertSame('/related_dummies/1', $iriConverter->getIriFromResource($item)); + $this->assertSame('/dummies/1', $iriConverter->getIriFromResource($item, UrlGeneratorInterface::ABS_PATH, null, ['item_uri_template' => '/unresolved'])); } public function testGetIriFromItemWithNoOperations(): void @@ -399,6 +423,24 @@ public function testGetNoItemFromIri(): void $iriConverter->getResourceFromIri('/dummies/1'); } + private function getMockedIriConverter(RouterInterface $router, ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, ?IdentifiersExtractorInterface $identifiersExtractor = null, ?ResourceClassResolverInterface $resourceClassResolver = null, ?OperationMetadataFactoryInterface $operationMetadataFactory = null): IriConverter + { + if (!$resourceClassResolver) { + $resourceClassResolver = $this->createStub(ResourceClassResolverInterface::class); + $resourceClassResolver->method('isResourceClass')->willReturn(true); + $resourceClassResolver->method('getResourceClass')->willReturnCallback(static fn (object $object): string => $object::class); + } + + return new IriConverter( + $this->createStub(ProviderInterface::class), + $router, + $identifiersExtractor ?? $this->createStub(IdentifiersExtractorInterface::class), + $resourceClassResolver, + $resourceMetadataCollectionFactory, + operationMetadataFactory: $operationMetadataFactory, + ); + } + private function getResourceClassResolver() { $resourceClassResolver = $this->prophesize(ResourceClassResolverInterface::class);