Skip to content

fix(serializer): empty object as array with supports cache #5100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
soyuka merged 4 commits into from
Nov 12, 2022
Merged

fix(serializer): empty object as array with supports cache #5100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd44a2c
fix(serializer): empty object as array with supports cache
soyuka Oct 21, 2022
077f1f2
fix issue when empty operation
soyuka Oct 24, 2022
5157d95
revert 08450c2684f82fa83c3d099de74ff225e82c1d3e
soyuka Oct 27, 2022
9667823
review
soyuka Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/Api/IdentifiersExtractor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,26 +49,34 @@ public function __construct(ResourceMetadataCollectionFactoryInterface $resource
*/
public function getIdentifiersFromItem(object $item, Operation $operation = null, array $context = []): array
{
$identifiers = [];

if (!$this->isResourceClass($this->getObjectClass($item))) {
return ['id' => $this->propertyAccessor->getValue($item, 'id')];
}

if ($operation && $operation->getClass()) {
return $this->getIdentifiersFromOperation($item, $operation, $context);
}

$resourceClass = $this->getResourceClass($item, true);
$operation ??= $this->resourceMetadataFactory->create($resourceClass)->getOperation(null, false, true);

return $this->getIdentifiersFromOperation($item, $operation, $context);
}

private function getIdentifiersFromOperation(object $item, Operation $operation, array $context = []): array
{
if ($operation instanceof HttpOperation) {
$links = $operation->getUriVariables();
} elseif ($operation instanceof GraphQlOperation) {
$links = $operation->getLinks();
}

$identifiers = [];
foreach ($links ?? [] as $link) {
if (1 < (is_countable($link->getIdentifiers()) ? \count($link->getIdentifiers()) : 0)) {
$compositeIdentifiers = [];
foreach ($link->getIdentifiers() as $identifier) {
$compositeIdentifiers[$identifier] = $this->getIdentifierValue($item, $link->getFromClass() ?? $resourceClass, $identifier, $link->getParameterName());
$compositeIdentifiers[$identifier] = $this->getIdentifierValue($item, $link->getFromClass() ?? $operation->getClass(), $identifier, $link->getParameterName());
}

$identifiers[$link->getParameterName()] = CompositeIdentifierParser::stringify($compositeIdentifiers);
Expand Down
12 changes: 9 additions & 3 deletions src/Hydra/Serializer/CollectionFiltersNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use Psr\Container\ContainerInterface;
use Symfony\Component\Serializer\Exception\UnexpectedValueException;
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;
use Symfony\Component\Serializer\Normalizer\CacheableSupportsMethodInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
Expand Down Expand Up @@ -58,13 +59,18 @@ public function hasCacheableSupportsMethod(): bool
*/
public function normalize(mixed $object, string $format = null, array $context = []): array|string|int|float|bool|\ArrayObject|null
{
$data = $this->collectionNormalizer->normalize($object, $format, $context);
if (!\is_array($data)) {
throw new UnexpectedValueException('Expected data to be an array');
if (($context[AbstractObjectNormalizer::PRESERVE_EMPTY_OBJECTS] ?? false) && $object instanceof \ArrayObject && !\count($object)) {
return $object;
}

$data = $this->collectionNormalizer->normalize($object, $format, $context);
if (!isset($context['resource_class']) || isset($context['api_sub_level'])) {
return $data;
}

if (!\is_array($data)) {
throw new UnexpectedValueException('Expected data to be an array');
}
$resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class']);
$operation = $context['operation'] ?? $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($context['operation_name'] ?? null);
$resourceFilters = $operation->getFilters();
Expand Down
28 changes: 25 additions & 3 deletions src/Hydra/Serializer/CollectionNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Serializer;

/**
* This normalizer handles collections.
Expand Down Expand Up @@ -56,16 +57,20 @@ public function __construct(private readonly ContextBuilderInterface $contextBui
*/
public function supportsNormalization(mixed $data, string $format = null, array $context = []): bool
{
return self::FORMAT === $format && is_iterable($data) && isset($context['resource_class']) && !isset($context['api_sub_level']);
return self::FORMAT === $format && is_iterable($data);
}

/**
* {@inheritdoc}
*
* @param iterable $object
*/
public function normalize(mixed $object, string $format = null, array $context = []): array
public function normalize(mixed $object, string $format = null, array $context = []): array|string|int|float|bool|\ArrayObject|null
{
if (!isset($context['resource_class']) || isset($context['api_sub_level'])) {
return $this->normalizeRawCollection($object, $format, $context);
}

$resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class']);
$context = $this->initContext($resourceClass, $context);
$context['api_collection_sub_level'] = true;
Expand Down Expand Up @@ -103,6 +108,23 @@ public function normalize(mixed $object, string $format = null, array $context =

public function hasCacheableSupportsMethod(): bool
{
return false;
return true;
}

/**
* Normalizes a raw collection (not API resources).
*/
protected function normalizeRawCollection(iterable $object, string $format = null, array $context = []): array|\ArrayObject
{
if (\is_array($object) && !$object && ($context[Serializer::EMPTY_ARRAY_AS_OBJECT] ?? false)) {
return new \ArrayObject();
}

$data = [];
foreach ($object as $index => $obj) {
$data[$index] = $this->normalizer->normalize($obj, $format, $context);
}

return $data;
}
}
7 changes: 4 additions & 3 deletions src/Hydra/Serializer/PartialCollectionViewNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,15 @@ public function __construct(private readonly NormalizerInterface $collectionNorm
public function normalize(mixed $object, string $format = null, array $context = []): array|string|int|float|bool|\ArrayObject|null
{
$data = $this->collectionNormalizer->normalize($object, $format, $context);
if (!\is_array($data)) {
throw new UnexpectedValueException('Expected data to be an array');
}

if (isset($context['api_sub_level'])) {
return $data;
}

if (!\is_array($data)) {
throw new UnexpectedValueException('Expected data to be an array');
}

$currentPage = $lastPage = $itemsPerPage = $pageTotalItems = null;
if ($paginated = ($object instanceof PartialPaginatorInterface)) {
if ($object instanceof PaginatorInterface) {
Expand Down
24 changes: 14 additions & 10 deletions src/Metadata/Resource/ResourceMetadataCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
*/
final class ResourceMetadataCollection extends \ArrayObject
{
private const GRAPHQL_PREFIX = 'g_';
private const HTTP_PREFIX = 'h_';
private array $operationCache = [];

public function __construct(private readonly string $resourceClass, array $input = [])
Expand All @@ -34,12 +36,12 @@ public function __construct(private readonly string $resourceClass, array $input
public function getOperation(?string $operationName = null, bool $forceCollection = false, bool $httpOperation = false): Operation
{
$operationName ??= '';
if (isset($this->operationCache[$operationName])) {
return $this->operationCache[$operationName];
if (isset($this->operationCache[self::HTTP_PREFIX.$operationName])) {
return $this->operationCache[self::HTTP_PREFIX.$operationName];
}

if (isset($this->operationCache['graphql_'.$operationName])) {
return $this->operationCache['graphql_'.$operationName];
if (isset($this->operationCache[self::GRAPHQL_PREFIX.$operationName])) {
return $this->operationCache[self::GRAPHQL_PREFIX.$operationName];
}

$it = $this->getIterator();
Expand All @@ -51,27 +53,29 @@ public function getOperation(?string $operationName = null, bool $forceCollectio

foreach ($metadata->getOperations() ?? [] as $name => $operation) {
$isCollection = $operation instanceof CollectionOperationInterface;
if ('' === $operationName && \in_array($operation->getMethod() ?? HttpOperation::METHOD_GET, [HttpOperation::METHOD_GET, HttpOperation::METHOD_OPTIONS, HttpOperation::METHOD_HEAD], true) && ($forceCollection ? $isCollection : !$isCollection)) {
return $this->operationCache[$operationName] = $operation;
$method = $operation->getMethod() ?? HttpOperation::METHOD_GET;
$isGetOperation = HttpOperation::METHOD_GET === $method || HttpOperation::METHOD_OPTIONS === $method || HttpOperation::METHOD_HEAD === $method;
if ('' === $operationName && $isGetOperation && ($forceCollection ? $isCollection : !$isCollection)) {

@alanpoulain alanpoulain Oct 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking this PR as an opportunity for a long time question I have: is adding an empty string as key in the operationCache a good idea?

@soyuka soyuka Oct 27, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to prefix these, but yeah the representation of null as string would be an empty string ?

@alanpoulain alanpoulain Oct 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to follow: if we ask a null $operationName to this getOperation method, it means we don't know the operation name. It it is cached, the same operation is always returned, even if the other conditions have changed ($isGetOperation, $forceCollection...). It seems dangerous to me.

@soyuka soyuka Nov 10, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix this in another patch then.

return $this->operationCache[self::HTTP_PREFIX.$operationName] = $operation;
}

if ($name === $operationName) {
return $this->operationCache[$operationName] = $operation;
return $this->operationCache[self::HTTP_PREFIX.$operationName] = $operation;
}

if ($operation->getUriTemplate() === $operationName) {
return $this->operationCache[$operationName] = $operation;
return $this->operationCache[self::HTTP_PREFIX.$operationName] = $operation;
}
}

foreach ($metadata->getGraphQlOperations() ?? [] as $name => $operation) {
$isCollection = $operation instanceof CollectionOperationInterface;
if ('' === $operationName && ($forceCollection ? $isCollection : !$isCollection) && false === $httpOperation) {
return $this->operationCache['graphql_'.$operationName] = $operation;
return $this->operationCache[self::GRAPHQL_PREFIX.$operationName] = $operation;
}

if ($name === $operationName) {
return $this->operationCache['graphql_'.$operationName] = $operation;
return $this->operationCache[self::GRAPHQL_PREFIX.$operationName] = $operation;
}
}

Expand Down
26 changes: 24 additions & 2 deletions src/Serializer/AbstractCollectionNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Serializer;

/**
* Base collection normalizer.
Expand Down Expand Up @@ -50,12 +51,12 @@ public function __construct(protected ResourceClassResolverInterface $resourceCl
*/
public function supportsNormalization(mixed $data, string $format = null, array $context = []): bool
{
return static::FORMAT === $format && is_iterable($data) && isset($context['resource_class']) && !isset($context['api_sub_level']);
return static::FORMAT === $format && is_iterable($data);
}

public function hasCacheableSupportsMethod(): bool
{
return false;
return true;
}

/**
Expand All @@ -65,6 +66,10 @@ public function hasCacheableSupportsMethod(): bool
*/
public function normalize(mixed $object, string $format = null, array $context = []): array|string|int|float|bool|\ArrayObject|null
{
if (!isset($context['resource_class']) || isset($context['api_sub_level'])) {
return $this->normalizeRawCollection($object, $format, $context);
}

$resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class']);
$context = $this->initContext($resourceClass, $context);
$data = [];
Expand All @@ -82,6 +87,23 @@ public function normalize(mixed $object, string $format = null, array $context =
return array_merge_recursive($data, $paginationData, $itemsData);
}

/**
* Normalizes a raw collection (not API resources).
*/
protected function normalizeRawCollection(iterable $object, string $format = null, array $context = []): array|\ArrayObject
{
if (!$object && ($context[Serializer::EMPTY_ARRAY_AS_OBJECT] ?? false) && \is_array($object)) {
return new \ArrayObject();
}

$data = [];
foreach ($object as $index => $obj) {
$data[$index] = $this->normalizer->normalize($obj, $format, $context);
}

return $data;
}

/**
* Gets the pagination configuration.
*/
Expand Down
30 changes: 22 additions & 8 deletions src/Serializer/AbstractItemNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
use Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Serializer;

/**
* Base item normalizer.
Expand All @@ -54,6 +55,7 @@ abstract class AbstractItemNormalizer extends AbstractObjectNormalizer

protected PropertyAccessorInterface $propertyAccessor;
protected array $localCache = [];
protected array $localFactoryOptionsCache = [];

public function __construct(protected PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, protected PropertyMetadataFactoryInterface $propertyMetadataFactory, protected IriConverterInterface $iriConverter, protected ResourceClassResolverInterface $resourceClassResolver, PropertyAccessorInterface $propertyAccessor = null, NameConverterInterface $nameConverter = null, ClassMetadataFactoryInterface $classMetadataFactory = null, array $defaultContext = [], ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory = null, protected ?ResourceAccessCheckerInterface $resourceAccessChecker = null)
{
Expand Down Expand Up @@ -504,23 +506,35 @@ protected function denormalizeRelation(string $attributeName, ApiProperty $prope
*/
protected function getFactoryOptions(array $context): array
{
$operationCacheKey = ($context['resource_class'] ?? '').($context['operation_name'] ?? '').($context['api_normalize'] ?? '');
if ($operationCacheKey && isset($this->localFactoryOptionsCache[$operationCacheKey])) {
return $this->localFactoryOptionsCache[$operationCacheKey];
}

$options = [];

if (isset($context[self::GROUPS])) {
/* @see https://github.com/symfony/symfony/blob/v4.2.6/src/Symfony/Component/PropertyInfo/Extractor/SerializerExtractor.php */
$options['serializer_groups'] = (array) $context[self::GROUPS];
}

if (isset($context['resource_class']) && $this->resourceClassResolver->isResourceClass($context['resource_class']) && $this->resourceMetadataCollectionFactory) {
$resourceClass = $this->resourceClassResolver->getResourceClass(null, $context['resource_class']); // fix for abstract classes and interfaces
// This is a hot spot, we should avoid calling this here but in many cases we can't
$operation = $context['operation'] ?? $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($context['operation_name'] ?? null);
$options['normalization_groups'] = $operation->getNormalizationContext()['groups'] ?? null;
$options['denormalization_groups'] = $operation->getDenormalizationContext()['groups'] ?? null;
$options['operation_name'] = $operation->getName();
// This is a hot spot
if (isset($context['resource_class'])) {
$operation = $context['operation'] ?? null;

if (!$operation && $this->resourceMetadataCollectionFactory && $this->resourceClassResolver->isResourceClass($context['resource_class'])) {
$resourceClass = $this->resourceClassResolver->getResourceClass(null, $context['resource_class']); // fix for abstract classes and interfaces
$operation = $this->resourceMetadataCollectionFactory->create($resourceClass)->getOperation($context['operation_name'] ?? null);
}

if ($operation) {
$options['normalization_groups'] = $operation->getNormalizationContext()['groups'] ?? null;
$options['denormalization_groups'] = $operation->getDenormalizationContext()['groups'] ?? null;
$options['operation_name'] = $operation->getName();
}
}

return $options;
return $this->localFactoryOptionsCache[$operationCacheKey] = $options;
}

/**
Expand Down
Loading