Skip to content
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.

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

feat(doctrine): stateOptions can handleLinks for query optimization #5732

Merged
merged 1 commit into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .php-cs-fixer.dist.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
'src/Core/Bridge/Symfony/Maker/Resources/skeleton',
'tests/Fixtures/app/var',
'docs/guides',
'docs/var',
])
->notPath('src/Symfony/Bundle/DependencyInjection/Configuration.php')
->notPath('src/Annotation/ApiFilter.php') // temporary
Expand Down
17 changes: 17 additions & 0 deletions features/doctrine/handle_links.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
Feature: Use a link handler to retrieve a resource

@createSchema
Scenario: Get collection
Given there are a few link handled dummies
When I send a "GET" request to "/link_handled_dummies"
Then the response status code should be 200
And the response should be in JSON
And the JSON node "hydra:totalItems" should be equal to 1

@createSchema
Scenario: Get item
Given there are a few link handled dummies
When I send a "GET" request to "/link_handled_dummies/1"
Then the response status code should be 200
And the response should be in JSON
And the JSON node "slug" should be equal to "foo"
58 changes: 49 additions & 9 deletions features/doctrine/separated_resource.feature
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,6 @@ Feature: Use state options to use an entity that is not a resource
And the response should be in JSON
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"

@!mongodb
@createSchema
Scenario: Get item
Given there are 5 separated entities
When I send a "GET" request to "/separated_entities/1"
Then the response status code should be 200
And the response should be in JSON
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"

soyuka marked this conversation as resolved.
Show resolved Hide resolved
@!mongodb
@createSchema
Scenario: Get all EntityClassAndCustomProviderResources
Expand All @@ -74,3 +65,52 @@ Feature: Use state options to use an entity that is not a resource
Given there are 1 separated entities
When I send a "GET" request to "/entityClassAndCustomProviderResources/1"
Then the response status code should be 200

@mongodb
@createSchema
Scenario: Get collection
Given there are 5 separated entities
When I send a "GET" request to "/separated_documents"
Then the response status code should be 200
And the response should be in JSON
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
Then the JSON should be valid according to this schema:
"""
{
"type": "object",
"properties": {
"@context": {"pattern": "^/contexts/SeparatedDocument"},
"@id": {"pattern": "^/separated_documents"},
"@type": {"pattern": "^hydra:Collection$"},
"hydra:member": {
"type": "array",
"items": {
"type": "object"
}
},
"hydra:totalItems": {"type":"number"},
"hydra:view": {
"type": "object"
}
}
}
"""

@mongodb
@createSchema
Scenario: Get ordered collection
Given there are 5 separated entities
When I send a "GET" request to "/separated_documents?order[value]=desc"
Then the response status code should be 200
And the response should be in JSON
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
And the JSON node "hydra:member[0].value" should be equal to "5"

@mongodb
@createSchema
Scenario: Get item
Given there are 5 separated entities
When I send a "GET" request to "/separated_documents/1"
Then the response status code should be 200
And the response should be in JSON
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
4 changes: 2 additions & 2 deletions src/Doctrine/Common/Filter/OrderFilterTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ public function getDescription(string $resourceClass): array
$description = [];

$properties = $this->getProperties();
if (null === $properties) {
$properties = array_fill_keys($this->getClassMetadata($resourceClass)->getFieldNames(), null);
if (null === $properties && $fieldNames = $this->getClassMetadata($resourceClass)->getFieldNames()) {
$properties = array_fill_keys($fieldNames, null);
}

foreach ($properties as $property => $propertyOptions) {
Expand Down
17 changes: 4 additions & 13 deletions src/Doctrine/Common/PropertyHelperTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

namespace ApiPlatform\Doctrine\Common;

use Doctrine\Persistence\ManagerRegistry;
use Doctrine\Persistence\Mapping\ClassMetadata;

/**
Expand All @@ -25,7 +24,10 @@
*/
trait PropertyHelperTrait
{
abstract protected function getManagerRegistry(): ManagerRegistry;
/**
* Gets class metadata for the given resource.
*/
abstract protected function getClassMetadata(string $resourceClass): ClassMetadata;

/**
* Determines whether the given property is mapped.
Expand Down Expand Up @@ -125,15 +127,4 @@ protected function getNestedMetadata(string $resourceClass, array $associations)

return $metadata;
}

/**
* Gets class metadata for the given resource.
*/
protected function getClassMetadata(string $resourceClass): ClassMetadata
{
return $this
->getManagerRegistry()
->getManagerForClass($resourceClass)
->getClassMetadata($resourceClass);
}
}
22 changes: 21 additions & 1 deletion src/Doctrine/Common/State/LinksHandlerTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,19 @@
namespace ApiPlatform\Doctrine\Common\State;

use ApiPlatform\Exception\OperationNotFoundException;
use ApiPlatform\Exception\RuntimeException;
use ApiPlatform\Metadata\Exception\RuntimeException;
use ApiPlatform\Metadata\GraphQl\Operation as GraphQlOperation;
use ApiPlatform\Metadata\GraphQl\Query;
use ApiPlatform\Metadata\HttpOperation;
use ApiPlatform\Metadata\Link;
use ApiPlatform\Metadata\Operation;
use ApiPlatform\Metadata\Resource\Factory\ResourceMetadataCollectionFactoryInterface;
use Psr\Container\ContainerInterface;

trait LinksHandlerTrait
{
private ?ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory;
private ?ContainerInterface $handleLinksLocator;

/**
* @return Link[]
Expand Down Expand Up @@ -112,4 +114,22 @@

return [];
}

private function getLinksHandler(Operation $operation): ?callable
{
if (!($options = $operation->getStateOptions()) || !method_exists($options, 'getHandleLinks') || null === $options->getHandleLinks()) {
return null;
}

$handleLinks = $options->getHandleLinks(); // @phpstan-ignore-line method_exists called above
if (\is_callable($handleLinks)) {
return $handleLinks;
}

if ($this->handleLinksLocator && \is_string($handleLinks) && $this->handleLinksLocator->has($handleLinks)) {
return [$this->handleLinksLocator->get($handleLinks), 'handleLinks'];

Check warning on line 130 in src/Doctrine/Common/State/LinksHandlerTrait.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Common/State/LinksHandlerTrait.php#L129-L130

Added lines #L129 - L130 were not covered by tests
}

throw new RuntimeException(sprintf('Could not find handleLinks service "%s"', $handleLinks));

Check warning on line 133 in src/Doctrine/Common/State/LinksHandlerTrait.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Common/State/LinksHandlerTrait.php#L133

Added line #L133 was not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

use ApiPlatform\Doctrine\Odm\State\CollectionProvider;
use ApiPlatform\Doctrine\Odm\State\ItemProvider;
use ApiPlatform\Doctrine\Odm\State\Options;
use ApiPlatform\Metadata\ApiResource;
use ApiPlatform\Metadata\CollectionOperationInterface;
use ApiPlatform\Metadata\DeleteOperationInterface;
Expand Down Expand Up @@ -44,7 +45,12 @@
if ($operations) {
/** @var Operation $operation */
foreach ($resourceMetadata->getOperations() as $operationName => $operation) {
if (!$this->managerRegistry->getManagerForClass($operation->getClass()) instanceof DocumentManager) {
$documentClass = $operation->getClass();
if (($options = $operation->getStateOptions()) && $options instanceof Options && $options->getDocumentClass()) {
$documentClass = $options->getDocumentClass();

Check warning on line 50 in src/Doctrine/Odm/Metadata/Resource/DoctrineMongoDbOdmResourceCollectionMetadataFactory.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/Metadata/Resource/DoctrineMongoDbOdmResourceCollectionMetadataFactory.php#L50

Added line #L50 was not covered by tests
}

if (!$this->managerRegistry->getManagerForClass($documentClass) instanceof DocumentManager) {
continue;
}

Expand Down
16 changes: 15 additions & 1 deletion src/Doctrine/Odm/PropertyHelperTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Doctrine\ODM\MongoDB\Aggregation\Builder;
use Doctrine\ODM\MongoDB\Mapping\ClassMetadata as MongoDbOdmClassMetadata;
use Doctrine\ODM\MongoDB\Mapping\MappingException;
use Doctrine\Persistence\ManagerRegistry;
use Doctrine\Persistence\Mapping\ClassMetadata;

/**
Expand All @@ -26,6 +27,8 @@
*/
trait PropertyHelperTrait
{
abstract protected function getManagerRegistry(): ManagerRegistry;

/**
* Splits the given property into parts.
*/
Expand All @@ -34,7 +37,18 @@
/**
* Gets class metadata for the given resource.
*/
abstract protected function getClassMetadata(string $resourceClass): ClassMetadata;
protected function getClassMetadata(string $resourceClass): ClassMetadata
{
$manager = $this
->getManagerRegistry()
->getManagerForClass($resourceClass);

if ($manager) {
return $manager->getClassMetadata($resourceClass);

Check warning on line 47 in src/Doctrine/Odm/PropertyHelperTrait.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/PropertyHelperTrait.php#L47

Added line #L47 was not covered by tests
}

return new MongoDbOdmClassMetadata($resourceClass);
}

/**
* Adds the necessary lookups for a nested property.
Expand Down
30 changes: 20 additions & 10 deletions src/Doctrine/Odm/State/CollectionProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Doctrine\ODM\MongoDB\DocumentManager;
use Doctrine\ODM\MongoDB\Repository\DocumentRepository;
use Doctrine\Persistence\ManagerRegistry;
use Psr\Container\ContainerInterface;

/**
* Collection state provider using the Doctrine ODM.
Expand All @@ -33,37 +34,46 @@
/**
* @param AggregationCollectionExtensionInterface[] $collectionExtensions
*/
public function __construct(ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly ManagerRegistry $managerRegistry, private readonly iterable $collectionExtensions = [])
public function __construct(ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly ManagerRegistry $managerRegistry, private readonly iterable $collectionExtensions = [], ContainerInterface $handleLinksLocator = null)

Check warning on line 37 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L37

Added line #L37 was not covered by tests
{
$this->resourceMetadataCollectionFactory = $resourceMetadataCollectionFactory;
$this->handleLinksLocator = $handleLinksLocator;

Check warning on line 40 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L40

Added line #L40 was not covered by tests
}

public function provide(Operation $operation, array $uriVariables = [], array $context = []): iterable
{
$resourceClass = $operation->getClass();
$documentClass = $operation->getClass();
if (($options = $operation->getStateOptions()) && $options instanceof Options && $options->getDocumentClass()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant with DoctrineMongoDbOdmResourceCollectionMetadataFactory, ODM/ItemProvider, ODM/LinksHandlerTrait, GraphQl/FieldsBuilder, Hydra/CollectionFiltersNormalizer, etc.

Can't it be in a more generic trait (e.g. Doctrine/Common/OptionsTrait) or something similar? (same with ORM)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a quite hard problem I'm not a huge fan of the current dependency between CollectionFiltersNormalizer and state options as when we'll need to subtree split we potentially won't have access to them. When working on this we probably need to uplift this information into some $context['filter_class'] as this option allows filters do be described on an entity instead of on a resource... This adds complexity whereas to where to put the Trait, and also would need specific implementations (ODM vs ORM). I'd like to keep this as-is for now.

Magic is hard to implement correctly :|.

$documentClass = $options->getDocumentClass();

Check warning on line 47 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L45-L47

Added lines #L45 - L47 were not covered by tests
}

/** @var DocumentManager $manager */
$manager = $this->managerRegistry->getManagerForClass($resourceClass);
$manager = $this->managerRegistry->getManagerForClass($documentClass);

Check warning on line 51 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L51

Added line #L51 was not covered by tests

$repository = $manager->getRepository($resourceClass);
$repository = $manager->getRepository($documentClass);

Check warning on line 53 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L53

Added line #L53 was not covered by tests
if (!$repository instanceof DocumentRepository) {
throw new RuntimeException(sprintf('The repository for "%s" must be an instance of "%s".', $resourceClass, DocumentRepository::class));
throw new RuntimeException(sprintf('The repository for "%s" must be an instance of "%s".', $documentClass, DocumentRepository::class));

Check warning on line 55 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L55

Added line #L55 was not covered by tests
}

$aggregationBuilder = $repository->createAggregationBuilder();

$this->handleLinks($aggregationBuilder, $uriVariables, $context, $resourceClass, $operation);
if ($handleLinks = $this->getLinksHandler($operation)) {
$handleLinks($aggregationBuilder, $uriVariables, ['documentClass' => $documentClass, 'operation' => $operation] + $context);

Check warning on line 61 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L60-L61

Added lines #L60 - L61 were not covered by tests
} else {
$this->handleLinks($aggregationBuilder, $uriVariables, $context, $documentClass, $operation);

Check warning on line 63 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L63

Added line #L63 was not covered by tests
}

foreach ($this->collectionExtensions as $extension) {
$extension->applyToCollection($aggregationBuilder, $resourceClass, $operation, $context);
$extension->applyToCollection($aggregationBuilder, $documentClass, $operation, $context);

Check warning on line 67 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L67

Added line #L67 was not covered by tests

if ($extension instanceof AggregationResultCollectionExtensionInterface && $extension->supportsResult($resourceClass, $operation, $context)) {
return $extension->getResult($aggregationBuilder, $resourceClass, $operation, $context);
if ($extension instanceof AggregationResultCollectionExtensionInterface && $extension->supportsResult($documentClass, $operation, $context)) {
return $extension->getResult($aggregationBuilder, $documentClass, $operation, $context);

Check warning on line 70 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L69-L70

Added lines #L69 - L70 were not covered by tests
}
}

$attribute = $operation->getExtraProperties()['doctrine_mongodb'] ?? [];
$executeOptions = $attribute['execute_options'] ?? [];

return $aggregationBuilder->hydrate($resourceClass)->execute($executeOptions);
return $aggregationBuilder->hydrate($documentClass)->execute($executeOptions);

Check warning on line 77 in src/Doctrine/Odm/State/CollectionProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/CollectionProvider.php#L77

Added line #L77 was not covered by tests
}
}
32 changes: 21 additions & 11 deletions src/Doctrine/Odm/State/ItemProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Doctrine\ODM\MongoDB\DocumentManager;
use Doctrine\ODM\MongoDB\Repository\DocumentRepository;
use Doctrine\Persistence\ManagerRegistry;
use Psr\Container\ContainerInterface;

/**
* Item state provider using the Doctrine ODM.
Expand All @@ -36,41 +37,50 @@
/**
* @param AggregationItemExtensionInterface[] $itemExtensions
*/
public function __construct(ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly ManagerRegistry $managerRegistry, private readonly iterable $itemExtensions = [])
public function __construct(ResourceMetadataCollectionFactoryInterface $resourceMetadataCollectionFactory, private readonly ManagerRegistry $managerRegistry, private readonly iterable $itemExtensions = [], ContainerInterface $handleLinksLocator = null)

Check warning on line 40 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L40

Added line #L40 was not covered by tests
{
$this->resourceMetadataCollectionFactory = $resourceMetadataCollectionFactory;
$this->handleLinksLocator = $handleLinksLocator;

Check warning on line 43 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L43

Added line #L43 was not covered by tests
}

public function provide(Operation $operation, array $uriVariables = [], array $context = []): ?object
{
$resourceClass = $operation->getClass();
$documentClass = $operation->getClass();
if (($options = $operation->getStateOptions()) && $options instanceof Options && $options->getDocumentClass()) {
$documentClass = $options->getDocumentClass();

Check warning on line 50 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L48-L50

Added lines #L48 - L50 were not covered by tests
}

/** @var DocumentManager $manager */
$manager = $this->managerRegistry->getManagerForClass($resourceClass);
$manager = $this->managerRegistry->getManagerForClass($documentClass);

Check warning on line 54 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L54

Added line #L54 was not covered by tests

$fetchData = $context['fetch_data'] ?? true;
if (!$fetchData) {
return $manager->getReference($resourceClass, reset($uriVariables));
return $manager->getReference($documentClass, reset($uriVariables));

Check warning on line 58 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L58

Added line #L58 was not covered by tests
}

$repository = $manager->getRepository($resourceClass);
$repository = $manager->getRepository($documentClass);

Check warning on line 61 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L61

Added line #L61 was not covered by tests
if (!$repository instanceof DocumentRepository) {
throw new RuntimeException(sprintf('The repository for "%s" must be an instance of "%s".', $resourceClass, DocumentRepository::class));
throw new RuntimeException(sprintf('The repository for "%s" must be an instance of "%s".', $documentClass, DocumentRepository::class));

Check warning on line 63 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L63

Added line #L63 was not covered by tests
}

$aggregationBuilder = $repository->createAggregationBuilder();

$this->handleLinks($aggregationBuilder, $uriVariables, $context, $resourceClass, $operation);
if ($handleLinks = $this->getLinksHandler($operation)) {
$handleLinks($aggregationBuilder, $uriVariables, ['documentClass' => $documentClass, 'operation' => $operation] + $context);

Check warning on line 69 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L68-L69

Added lines #L68 - L69 were not covered by tests
} else {
$this->handleLinks($aggregationBuilder, $uriVariables, $context, $documentClass, $operation);

Check warning on line 71 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L71

Added line #L71 was not covered by tests
}

foreach ($this->itemExtensions as $extension) {
$extension->applyToItem($aggregationBuilder, $resourceClass, $uriVariables, $operation, $context);
$extension->applyToItem($aggregationBuilder, $documentClass, $uriVariables, $operation, $context);

Check warning on line 75 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L75

Added line #L75 was not covered by tests

if ($extension instanceof AggregationResultItemExtensionInterface && $extension->supportsResult($resourceClass, $operation, $context)) {
return $extension->getResult($aggregationBuilder, $resourceClass, $operation, $context);
if ($extension instanceof AggregationResultItemExtensionInterface && $extension->supportsResult($documentClass, $operation, $context)) {
return $extension->getResult($aggregationBuilder, $documentClass, $operation, $context);

Check warning on line 78 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L77-L78

Added lines #L77 - L78 were not covered by tests
}
}

$executeOptions = $operation->getExtraProperties()['doctrine_mongodb']['execute_options'] ?? [];

return $aggregationBuilder->hydrate($resourceClass)->execute($executeOptions)->current() ?: null;
return $aggregationBuilder->hydrate($documentClass)->execute($executeOptions)->current() ?: null;

Check warning on line 84 in src/Doctrine/Odm/State/ItemProvider.php

View check run for this annotation

Codecov / codecov/patch

src/Doctrine/Odm/State/ItemProvider.php#L84

Added line #L84 was not covered by tests
}
}
Loading
Loading