IriConverter: localOperationCache is always written but never read

Open Beginner friendly
#8,471 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
85/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
php, symfony

Research direction

Start in src/Symfony/Routing/IriConverter.php, focusing on getIriFromResource() and the local operation-cache lookup around the metadata-factory call. Review IriConverterTest::testGetIriFromItemWithoutOperation, add coverage proving repeated calls reuse the cache, and run the IriConverter test suite to verify the expensive metadata lookup is avoided.

Written by the indexing model from the issue text.

Description

API Platform version(s) affected: 4.3.3

Description
In a getCollection, the AbstractItemNormalizer will use the IriConverter to find the IRIs for all entities in the collection, including related entities. To find these IRI's, the resource metadata has to be created to find what operation to use to generate the IRI. This resource metadata is cached in the IriConverter, but will only be read if an $operation was specified. The AbstractItemNormalizer does not specify this, so the cache is always written but never read.

This is the relevant method in ApiPlatform\Symfony\Routing\IriConverter:

public function getIriFromResource(object|string $resource, int $referenceType = UrlGeneratorInterface::ABS_PATH, ?Operation $operation = null, array $context = []): string
    {
        $resourceClass = $context['force_resource_class'] ?? (\is_string($resource) ? $resource : $this->getObjectClass($resource));

        if ($this->operationMetadataFactory && isset($context['item_uri_template'])) {
            $operation = $this->operationMetadataFactory->create($context['item_uri_template']);
        }

        $localOperationCacheKey = ($operation?->getName() ?? '').$resourceClass.(\is_string($resource) ? '_s' : '_o').($operation instanceof CollectionOperationInterface ? '_c' : '_i');
        // ❗️This short-circuits on $operation, so if no $operation is present the localOperationCache will never be used
        if ($operation && isset($this->localOperationCache[$localOperationCacheKey])) {
            return $this->generateSymfonyRoute($resource, $referenceType, $this->localOperationCache[$localOperationCacheKey], $context, $this->localIdentifiersExtractorOperationCache[$localOperationCacheKey] ?? null);
        }

        if (!($isResourceClass = $this->resourceClassResolver->isResourceClass($resourceClass)) && !isset($context['item_uri_template'])) {
            return $this->generateSkolemIri($resource, $referenceType, $operation, $context, $resourceClass);
        }

        $context['is_resource_class'] = $isResourceClass;
        $context['current_resource_class'] = $resourceClass;

        // This is only for when a class (that is not a resource) extends another one that is a resource, we should remove this behavior
        if (!\is_string($resource) && !isset($context['force_resource_class']) && !isset($context['item_uri_template'])) {
            $resourceClass = $this->getResourceClass($resource, true);
        }

        if (!$operation) {
            $operation = (new Get())->withClass($resourceClass);
        }

        if ($operation instanceof HttpOperation && 301 === $operation->getStatus()) {
            $operation = ($operation instanceof CollectionOperationInterface ? new GetCollection() : new Get())->withClass($operation->getClass());
            unset($context['uri_variables']);
        }

        $identifiersExtractorOperation = $operation;
        // In symfony the operation name is the route name, try to find one if none provided
        if (
            !$operation->getName()
            || ($operation instanceof HttpOperation && 'POST' === $operation->getMethod())
        ) {
            $forceCollection = $operation instanceof CollectionOperationInterface;
            try {
                // ❗️This is the actual expensive call
                $operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation(null, $forceCollection, true);
                $identifiersExtractorOperation = $operation;
            } catch (OperationNotFoundException) {
            }
        }

        if (!$operation->getName() || ($operation instanceof HttpOperation && $operation->getUriTemplate() && str_starts_with($operation->getUriTemplate(), SkolemIriConverter::$skolemUriTemplate))) {
            return $this->generateSkolemIri($resource, $referenceType, $operation, $context, $resourceClass);
        }

        $this->localOperationCache[$localOperationCacheKey] = $operation;
        $this->localIdentifiersExtractorOperationCache[$localOperationCacheKey] = $identifiersExtractorOperation;

        return $this->generateSymfonyRoute($resource, $referenceType, $operation, $context, $identifiersExtractorOperation);
    }

How to reproduce
IriConverterTest::testGetIriFromItemWithoutOperation touches on the logic that's affected by this bug, but does not assert the cache was used.

Possible Solution
Do not check if $operation is truthy before trying to read the cache:

--- a/src/Symfony/Routing/IriConverter.php
+++ b/src/Symfony/Routing/IriConverter.php
@@ -160,10 +160,15 @@
             !$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) {
+                }   
             }   
         }       

Additional Context
Entry points:

  • for each item's own '@id': JsonLd\Serializer\ItemNormalizer::normalize():121 does try to supply
    one via $context['operation'] ?? null, but the context no longer has it —
    AbstractCollectionNormalizer builds the per-item context through createOperationContext(),
    which ends at OperationContextTrait.php:53 with
    unset($context['operation'], $context['operation_name']);.
  • for related objects: AbstractItemNormalizer::normalizeRelation():982 passes only two named arguments,
    so $operation defaults to null:
    $context['iri'] = $iri = $this->iriConverter->getIriFromResource(resource: $relatedObject, context: $context);
    

On a real endpoint, 2,337 items with ~6 IRIs each, Xdebug profile, PHP 8.4.20, Symfony 7.4.8, api-platform/core 4.3.3: getIriFromResource() is called 14,362 times, and fell through to $this->resourceMetadataCollectionFactory->create 14,360 times.

Dominant language
PHP
Stars
2.6k
Forks
982
Avg merge
1d 23h
Merged PRs (30d)
42

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from api-platform/core

All issues in api-platform/core

Similar issues

More PHP issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.