From 748964285fda30a80c6adfdd6a991d813ef46c57 Mon Sep 17 00:00:00 2001 From: Teoh Han Hui Date: Mon, 30 Mar 2020 19:44:44 +0200 Subject: [PATCH] Fix behaviour of "order" attribute --- .../MongoDbOdm/Extension/OrderExtension.php | 62 +++++++---- .../Doctrine/Orm/Extension/OrderExtension.php | 105 ++++++++++++++---- .../Bundle/Resources/config/doctrine_orm.xml | 1 + .../Extension/OrderExtensionTest.php | 43 ++++--- .../Orm/Extension/OrderExtensionTest.php | 57 +++++----- 5 files changed, 173 insertions(+), 95 deletions(-) diff --git a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php index d335235aff4..c01f97872cd 100644 --- a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php +++ b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php @@ -17,7 +17,7 @@ use ApiPlatform\Core\Bridge\Doctrine\MongoDbOdm\PropertyHelperTrait as MongoDbOdmPropertyHelperTrait; use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; use Doctrine\Common\Persistence\ManagerRegistry; -use Doctrine\ODM\MongoDB\Aggregation\Builder; +use Doctrine\ODM\MongoDB\Aggregation\Builder as AggregationBuilder; /** * Applies selected ordering while querying resource collection. @@ -31,8 +31,11 @@ */ final class OrderExtension implements AggregationCollectionExtensionInterface { - use PropertyHelperTrait; use MongoDbOdmPropertyHelperTrait; + use PropertyHelperTrait; + + public const DIRECTION_ASC = 'ASC'; + public const DIRECTION_DESC = 'DESC'; private $order; private $resourceMetadataFactory; @@ -40,33 +43,42 @@ final class OrderExtension implements AggregationCollectionExtensionInterface public function __construct(string $order = null, ResourceMetadataFactoryInterface $resourceMetadataFactory = null, ManagerRegistry $managerRegistry = null) { - $this->resourceMetadataFactory = $resourceMetadataFactory; $this->order = $order; + $this->resourceMetadataFactory = $resourceMetadataFactory; $this->managerRegistry = $managerRegistry; } /** * {@inheritdoc} */ - public function applyToCollection(Builder $aggregationBuilder, string $resourceClass, string $operationName = null, array &$context = []) + public function applyToCollection(AggregationBuilder $aggregationBuilder, string $resourceClass, string $operationName = null, array &$context = []) { - $classMetaData = $this->getClassMetadata($resourceClass); - $identifiers = $classMetaData->getIdentifier(); if (null !== $this->resourceMetadataFactory) { - $defaultOrder = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order'); - if (null !== $defaultOrder) { - foreach ($defaultOrder as $field => $order) { - if (\is_int($field)) { - // Default direction - $field = $order; - $order = 'ASC'; + if (null !== $order = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order')) { + if (\is_string($order)) { + $direction = strtoupper($order); + if (\in_array($direction, [self::DIRECTION_ASC, self::DIRECTION_DESC], true)) { + $this->sortByAllIdentifiers($aggregationBuilder, $resourceClass, $direction, $context); + + return; } - if ($this->isPropertyNested($field, $resourceClass)) { - [$field] = $this->addLookupsForNestedProperty($field, $aggregationBuilder, $resourceClass); + $order = [$order]; + } + + foreach ($order as $property => $direction) { + if (\is_int($property)) { + $property = $direction; + $direction = self::DIRECTION_ASC; + } + + $field = $property; + + if ($this->isPropertyNested($property, $resourceClass)) { + [$field] = $this->addLookupsForNestedProperty($property, $aggregationBuilder, $resourceClass); } $aggregationBuilder->sort( - $context['mongodb_odm_sort_fields'] = ($context['mongodb_odm_sort_fields'] ?? []) + [$field => $order] + $context['mongodb_odm_sort_fields'] = ($context['mongodb_odm_sort_fields'] ?? []) + [$field => $direction] ); } @@ -75,16 +87,24 @@ public function applyToCollection(Builder $aggregationBuilder, string $resourceC } if (null !== $this->order) { - foreach ($identifiers as $identifier) { - $aggregationBuilder->sort( - $context['mongodb_odm_sort_fields'] = ($context['mongodb_odm_sort_fields'] ?? []) + [$identifier => $this->order] - ); - } + $this->sortByAllIdentifiers($aggregationBuilder, $resourceClass, $this->order, $context); } } + /** + * {@inheritdoc} + */ protected function getManagerRegistry(): ManagerRegistry { return $this->managerRegistry; } + + private function sortByAllIdentifiers(AggregationBuilder $aggregationBuilder, string $resourceClass, string $direction, array &$context): void + { + foreach ($this->getClassMetadata($resourceClass)->getIdentifier() as $field) { + $aggregationBuilder->sort( + $context['mongodb_odm_sort_fields'] = ($context['mongodb_odm_sort_fields'] ?? []) + [$field => $direction] + ); + } + } } diff --git a/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php b/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php index 1d2b1e3f469..e8d08c7d9cd 100644 --- a/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php +++ b/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php @@ -13,10 +13,14 @@ namespace ApiPlatform\Core\Bridge\Doctrine\Orm\Extension; -use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryBuilderHelper; +use ApiPlatform\Core\Bridge\Doctrine\Common\PropertyHelperTrait; +use ApiPlatform\Core\Bridge\Doctrine\Orm\PropertyHelperTrait as OrmPropertyHelperTrait; use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface; use ApiPlatform\Core\Exception\InvalidArgumentException; use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; +use Doctrine\Common\Persistence\ManagerRegistry; +use Doctrine\Common\Persistence\Mapping\ClassMetadata; +use Doctrine\ORM\EntityManager; use Doctrine\ORM\QueryBuilder; /** @@ -28,13 +32,25 @@ */ final class OrderExtension implements ContextAwareQueryCollectionExtensionInterface { + use OrmPropertyHelperTrait; + use PropertyHelperTrait; + + public const DIRECTION_ASC = 'ASC'; + public const DIRECTION_DESC = 'DESC'; + private $order; private $resourceMetadataFactory; + private $managerRegistry; + /** + * @var EntityManager|null + */ + private $entityManager; - public function __construct(string $order = null, ResourceMetadataFactoryInterface $resourceMetadataFactory = null) + public function __construct(string $order = null, ResourceMetadataFactoryInterface $resourceMetadataFactory = null, ManagerRegistry $managerRegistry = null) { - $this->resourceMetadataFactory = $resourceMetadataFactory; $this->order = $order; + $this->resourceMetadataFactory = $resourceMetadataFactory; + $this->managerRegistry = $managerRegistry; } /** @@ -46,29 +62,37 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator throw new InvalidArgumentException('The "$resourceClass" parameter must not be null'); } - $rootAlias = $queryBuilder->getRootAliases()[0]; + // BC + if (null === $this->managerRegistry) { + $this->entityManager = $queryBuilder->getEntityManager(); + } - $classMetaData = $queryBuilder->getEntityManager()->getClassMetadata($resourceClass); - $identifiers = $classMetaData->getIdentifier(); if (null !== $this->resourceMetadataFactory) { - $defaultOrder = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order'); - if (null !== $defaultOrder) { - foreach ($defaultOrder as $field => $order) { - if (\is_int($field)) { - // Default direction - $field = $order; - $order = 'ASC'; + if (null !== $order = $this->resourceMetadataFactory->create($resourceClass)->getAttribute('order')) { + if (\is_string($order)) { + $direction = strtoupper($order); + if (\in_array($direction, [self::DIRECTION_ASC, self::DIRECTION_DESC], true)) { + $this->addOrderByForAllIdentifiers($queryBuilder, $resourceClass, $direction); + + return; } - $pos = strpos($field, '.'); - if (false === $pos || isset($classMetaData->embeddedClasses[substr($field, 0, $pos)])) { - // Configure default filter with property - $field = "{$rootAlias}.{$field}"; - } else { - $alias = QueryBuilderHelper::addJoinOnce($queryBuilder, $queryNameGenerator, $rootAlias, substr($field, 0, $pos)); - $field = sprintf('%s.%s', $alias, substr($field, $pos + 1)); + $order = [$order]; + } + + foreach ($order as $property => $direction) { + if (\is_int($property)) { + $property = $direction; + $direction = self::DIRECTION_ASC; + } + + $alias = $queryBuilder->getRootAliases()[0]; + $field = $property; + + if ($this->isPropertyNested($property, $resourceClass)) { + [$alias, $field] = $this->addJoinsForNestedProperty($property, $alias, $queryBuilder, $queryNameGenerator, $resourceClass); } - $queryBuilder->addOrderBy($field, $order); + $queryBuilder->addOrderBy(sprintf('%s.%s', $alias, $field), $direction); } return; @@ -76,9 +100,44 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator } if (null !== $this->order) { - foreach ($identifiers as $identifier) { - $queryBuilder->addOrderBy("{$rootAlias}.{$identifier}", $this->order); + $this->addOrderByForAllIdentifiers($queryBuilder, $resourceClass, $this->order); + } + } + + /** + * {@inheritdoc} + */ + protected function getClassMetadata(string $resourceClass): ClassMetadata + { + // BC + if (null === $this->managerRegistry) { + if (null === $this->entityManager) { + throw new \RuntimeException('The manager registry was not provided, and the entity manager was not set on the class.'); } + + return $this->entityManager->getClassMetadata($resourceClass); + } + + return $this + ->getManagerRegistry() + ->getManagerForClass($resourceClass) + ->getClassMetadata($resourceClass); + } + + /** + * {@inheritdoc} + */ + protected function getManagerRegistry(): ManagerRegistry + { + return $this->managerRegistry; + } + + private function addOrderByForAllIdentifiers(QueryBuilder $queryBuilder, string $resourceClass, string $direction): void + { + $rootAlias = $queryBuilder->getRootAliases()[0]; + + foreach ($this->getClassMetadata($resourceClass)->getIdentifier() as $field) { + $queryBuilder->addOrderBy(sprintf('%s.%s', $rootAlias, $field), $direction); } } } diff --git a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml index ad6301b2bdf..4b8d5dc0a0d 100644 --- a/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml +++ b/src/Bridge/Symfony/Bundle/Resources/config/doctrine_orm.xml @@ -166,6 +166,7 @@ %api_platform.collection.order% + diff --git a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php index 1bb035e7278..3e844aede2b 100644 --- a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php @@ -118,37 +118,36 @@ public function testApplyToCollectionWithOrderOverriddenWithNoDirection() $orderExtensionTest->applyToCollection($aggregationBuilder, Dummy::class); } - public function testApplyToCollectionWithOrderOverriddenWithAssociation() + public function testApplyToCollectionOrderByNestedAssociationField(): void { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); - $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $resourceMetadataFactoryProphecy->create(Dummy::class)->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['author.name']])); + + $classMetadataProphecy = $this->prophesize(ClassMetadata::class); + $classMetadataProphecy->getIdentifier()->willReturn(['name']); + $classMetadataProphecy->hasAssociation('author')->willReturn(true); + $classMetadataProphecy->hasAssociation('name')->willReturn(false); + $classMetadataProphecy->getAssociationTargetClass('author')->willReturn(Dummy::class); + $classMetadataProphecy->hasReference('author')->willReturn(true); + $classMetadataProphecy->getFieldMapping('author')->willReturn(['isOwningSide' => true, 'storeAs' => ClassMetadata::REFERENCE_STORE_AS_ID]); + + $objectManagerProphecy = $this->prophesize(DocumentManager::class); + $objectManagerProphecy->getClassMetadata(Dummy::class)->willReturn($classMetadataProphecy); + + $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class); + $managerRegistryProphecy->getManagerForClass(Dummy::class)->willReturn($objectManagerProphecy); $lookupProphecy = $this->prophesize(Lookup::class); $lookupProphecy->localField('author')->shouldBeCalled()->willReturn($lookupProphecy); $lookupProphecy->foreignField('_id')->shouldBeCalled()->willReturn($lookupProphecy); $lookupProphecy->alias('author_lkup')->shouldBeCalled(); - $aggregationBuilderProphecy->lookup(Dummy::class)->shouldBeCalled()->willReturn($lookupProphecy->reveal()); + + $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $aggregationBuilderProphecy->lookup(Dummy::class)->shouldBeCalled()->willReturn($lookupProphecy); $aggregationBuilderProphecy->unwind('$author_lkup')->shouldBeCalled(); $aggregationBuilderProphecy->sort(['author_lkup.name' => 'ASC'])->shouldBeCalled(); - $classMetadataProphecy = $this->prophesize(ClassMetadata::class); - $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn(['name']); - $classMetadataProphecy->hasAssociation('author')->shouldBeCalled()->willReturn(true); - $classMetadataProphecy->hasAssociation('name')->shouldBeCalled()->willReturn(false); - $classMetadataProphecy->getAssociationTargetClass('author')->shouldBeCalled()->willReturn(Dummy::class); - $classMetadataProphecy->hasReference('author')->shouldBeCalled()->willReturn(true); - $classMetadataProphecy->getFieldMapping('author')->shouldBeCalled()->willReturn(['isOwningSide' => true, 'storeAs' => ClassMetadata::REFERENCE_STORE_AS_ID]); - - $resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['author.name']])); - - $objectManagerProphecy = $this->prophesize(DocumentManager::class); - $objectManagerProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); - - $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class); - $managerRegistryProphecy->getManagerForClass(Dummy::class)->shouldBeCalled()->willReturn($objectManagerProphecy->reveal()); - - $aggregationBuilder = $aggregationBuilderProphecy->reveal(); - $orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal(), $managerRegistryProphecy->reveal()); - $orderExtensionTest->applyToCollection($aggregationBuilder, Dummy::class); + $orderExtension = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal(), $managerRegistryProphecy->reveal()); + $orderExtension->applyToCollection($aggregationBuilderProphecy->reveal(), Dummy::class); } } diff --git a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php index e4f0478bd26..cd654ef0428 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php @@ -116,51 +116,50 @@ public function testApplyToCollectionWithOrderOverriddenWithNoDirection() $orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class); } - public function testApplyToCollectionWithOrderOverriddenWithAssociation() + public function testApplyToCollectionOrderByNestedAssociationField(): void { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); - $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); - - $queryBuilderProphecy->getDQLPart('join')->willReturn(['o' => []])->shouldBeCalled(); - $queryBuilderProphecy->innerJoin('o.author', 'author_a1', null, null)->shouldBeCalled(); - $queryBuilderProphecy->addOrderBy('author_a1.name', 'ASC')->shouldBeCalled(); + $resourceMetadataFactoryProphecy->create(Dummy::class)->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['author.name']])); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); - $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn(['name']); - - $resourceMetadataFactoryProphecy->create(Dummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['author.name']])); + $classMetadataProphecy->getIdentifier()->willReturn(['name']); + $classMetadataProphecy->hasAssociation('author')->willReturn(true); + $classMetadataProphecy->hasAssociation('name')->willReturn(false); + $classMetadataProphecy->getAssociationTargetClass('author')->willReturn(Dummy::class); - $emProphecy = $this->prophesize(EntityManager::class); - $emProphecy->getClassMetadata(Dummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); + $entityManagerProphecy = $this->prophesize(EntityManager::class); + $entityManagerProphecy->getClassMetadata(Dummy::class)->willReturn($classMetadataProphecy); - $queryBuilderProphecy->getEntityManager()->shouldBeCalled()->willReturn($emProphecy->reveal()); - $queryBuilderProphecy->getRootAliases()->shouldBeCalled()->willReturn(['o']); + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('join')->willReturn(['o' => []]); + $queryBuilderProphecy->innerJoin('o.author', 'author_a1', null, null)->shouldBeCalled(); + $queryBuilderProphecy->addOrderBy('author_a1.name', 'ASC')->shouldBeCalled(); + $queryBuilderProphecy->getEntityManager()->willReturn($entityManagerProphecy); + $queryBuilderProphecy->getRootAliases()->willReturn(['o']); - $queryBuilder = $queryBuilderProphecy->reveal(); - $orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal()); - $orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class); + $orderExtension = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal()); + $orderExtension->applyToCollection($queryBuilderProphecy->reveal(), new QueryNameGenerator(), Dummy::class); } - public function testApplyToCollectionWithOrderOverriddenWithEmbeddedAssociation() + public function testApplyToCollectionOrderByEmbeddedField(): void { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); - $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); - $queryBuilderProphecy->getRootAliases()->willReturn(['o']); - $queryBuilderProphecy->addOrderBy('o.embeddedDummy.dummyName', 'DESC')->shouldBeCalled(); + $resourceMetadataFactoryProphecy->create(EmbeddedDummy::class)->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['embeddedDummy.dummyName' => 'DESC']])); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); - $classMetadataProphecy->getIdentifier()->shouldBeCalled()->willReturn(['id']); + $classMetadataProphecy->getIdentifier()->willReturn(['id']); $classMetadataProphecy->embeddedClasses = ['embeddedDummy' => []]; + $classMetadataProphecy->hasAssociation('embeddedDummy')->willReturn(false); - $resourceMetadataFactoryProphecy->create(EmbeddedDummy::class)->shouldBeCalled()->willReturn(new ResourceMetadata(null, null, null, null, null, ['order' => ['embeddedDummy.dummyName' => 'DESC']])); - - $emProphecy = $this->prophesize(EntityManager::class); - $emProphecy->getClassMetadata(EmbeddedDummy::class)->shouldBeCalled()->willReturn($classMetadataProphecy->reveal()); + $entityManagerProphecy = $this->prophesize(EntityManager::class); + $entityManagerProphecy->getClassMetadata(EmbeddedDummy::class)->willReturn($classMetadataProphecy); - $queryBuilderProphecy->getEntityManager()->shouldBeCalled()->willReturn($emProphecy->reveal()); + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getRootAliases()->willReturn(['o']); + $queryBuilderProphecy->addOrderBy('o.embeddedDummy.dummyName', 'DESC')->shouldBeCalled(); + $queryBuilderProphecy->getEntityManager()->willReturn($entityManagerProphecy); - $queryBuilder = $queryBuilderProphecy->reveal(); - $orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal()); - $orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), EmbeddedDummy::class); + $orderExtension = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal()); + $orderExtension->applyToCollection($queryBuilderProphecy->reveal(), new QueryNameGenerator(), EmbeddedDummy::class); } }