api-platform / api-platform/core

IriConverter: localOperationCache is always written but never read

Abierto Apto para principiantes
#8,471 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
PHP
Estrellas
2.6k
Forks
980
Merge medio
2 d 4 h
PR fusionados (30 d)
49

Descripción

**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.

Guía de contribución

Abrir la guía de contribución

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
php, symfony
Área
api, backend, performance
Tipo de issue
Error
Dificultad
2/5
Tiempo estimado
1-3 horas
Estado de actividad
Activo
Claridad
Bien especificado
Aptitud para principiantes
85/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.