api-platform / api-platform/core

IriConverter: localOperationCache is always written but never read

Aberta Para iniciantes
#8,471 0 comentários 0 reações 0 responsáveis Ver no GitHub
Linguagem predominante
PHP
Estrelas
2.6k
Forks
980
Merge médio
2d 2h
PRs com merge (30d)
51

Descrição

**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`:
```php
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:

```diff
--- 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`:
```php
$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.

Guia de contribuição

Abrir o guia de contribuição

Direção de pesquisa

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.

Escrita pelo modelo de indexação a partir do texto da issue.

Avaliação

Stack de tecnologia
php, symfony
Domínio
api, backend, performance
Tipo de issue
Bug
Dificuldade
2/5
Tempo estimado
1-3 horas
Status de atividade
Ativa
Clareza
Claramente especificada
Facilidade para iniciantes
85/100

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.