From 42173a09c42bfc34498ab2dd51e91b40448c057a Mon Sep 17 00:00:00 2001 From: natepage Date: Fri, 8 Oct 2021 01:20:45 +1100 Subject: [PATCH 1/4] Do not apply order extension if sorting already applied --- .../MongoDbOdm/Extension/OrderExtension.php | 30 +++++++++++++++++++ .../Doctrine/Orm/Extension/OrderExtension.php | 6 ++++ .../Extension/OrderExtensionTest.php | 21 +++++++++++++ .../Orm/Extension/OrderExtensionTest.php | 14 +++++++++ 4 files changed, 71 insertions(+) diff --git a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php index 561597a4599..71fcd2af2e5 100644 --- a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php +++ b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php @@ -17,7 +17,9 @@ use ApiPlatform\Core\Bridge\Doctrine\MongoDbOdm\PropertyHelperTrait as MongoDbOdmPropertyHelperTrait; use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; use Doctrine\ODM\MongoDB\Aggregation\Builder; +use Doctrine\ODM\MongoDB\Aggregation\Stage\Sort; use Doctrine\Persistence\ManagerRegistry; +use OutOfRangeException; /** * Applies selected ordering while querying resource collection. @@ -50,6 +52,11 @@ public function __construct(string $order = null, ResourceMetadataFactoryInterfa */ public function applyToCollection(Builder $aggregationBuilder, string $resourceClass, string $operationName = null, array &$context = []) { + // Do not apply order if already defined on $aggregationBuilder + if ($this->hasSortStage($aggregationBuilder)) { + return; + } + $classMetaData = $this->getClassMetadata($resourceClass); $identifiers = $classMetaData->getIdentifier(); if (null !== $this->resourceMetadataFactory) { @@ -91,4 +98,27 @@ protected function getManagerRegistry(): ManagerRegistry { return $this->managerRegistry; } + + private function hasSortStage(Builder $aggregationBuilder): bool + { + $shouldStop = false; + $index = 0; + + do { + try { + if ($aggregationBuilder->getStage($index) instanceof Sort) { + // If at least one stage is sort, then it has sorting + return true; + } + } catch (OutOfRangeException $outOfRangeException) { + // There is no more stages on the aggregation builder + $shouldStop = true; + } + + $index++; + } while (!$shouldStop); + + // No stage was sort, and we iterated through all stages + return false; + } } diff --git a/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php b/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php index b9486857dea..355fe6fd657 100644 --- a/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php +++ b/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php @@ -46,6 +46,12 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator throw new InvalidArgumentException('The "$resourceClass" parameter must not be null'); } + // Do not apply order if already defined on queryBuilder + $orderByDqlPart = $queryBuilder->getDQLPart('orderBy'); + if (\is_array($orderByDqlPart) && \count($orderByDqlPart) > 0) { + return; + } + $rootAlias = $queryBuilder->getRootAliases()[0]; $classMetaData = $queryBuilder->getEntityManager()->getClassMetadata($resourceClass); diff --git a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php index ac38a38c335..639e2404fd8 100644 --- a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php @@ -20,10 +20,12 @@ use ApiPlatform\Core\Tests\ProphecyTrait; use Doctrine\ODM\MongoDB\Aggregation\Builder; use Doctrine\ODM\MongoDB\Aggregation\Stage\Lookup; +use Doctrine\ODM\MongoDB\Aggregation\Stage\Sort; use Doctrine\ODM\MongoDB\DocumentManager; use Doctrine\ODM\MongoDB\Mapping\ClassMetadata; use Doctrine\Persistence\ManagerRegistry; use PHPUnit\Framework\TestCase; +use \OutOfRangeException; /** * @group mongodb @@ -38,6 +40,7 @@ public function testApplyToCollectionWithValidOrder() { $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['name' => 'asc'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -58,6 +61,7 @@ public function testApplyToCollectionWithWrongOrder() { $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['name' => 'asc'])->shouldNotBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -79,6 +83,7 @@ public function testApplyToCollectionWithOrderOverridden() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['foo' => 'DESC'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -102,6 +107,7 @@ public function testApplyToCollectionWithOrderOverriddenWithNoDirection() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $aggregationBuilderProphecy = $this->prophesize(Builder::class); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['foo' => 'ASC'])->shouldBeCalled(); $aggregationBuilderProphecy->sort(['foo' => 'ASC', 'bar' => 'DESC'])->shouldBeCalled(); @@ -132,6 +138,7 @@ public function testApplyToCollectionWithOrderOverriddenWithAssociation() $lookupProphecy->alias('author_lkup')->shouldBeCalled(); $aggregationBuilderProphecy->lookup(Dummy::class)->shouldBeCalled()->willReturn($lookupProphecy->reveal()); $aggregationBuilderProphecy->unwind('$author_lkup')->shouldBeCalled(); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['author_lkup.name' => 'ASC'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -154,4 +161,18 @@ public function testApplyToCollectionWithOrderOverriddenWithAssociation() $orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal(), $managerRegistryProphecy->reveal()); $orderExtensionTest->applyToCollection($aggregationBuilder, Dummy::class); } + + public function testApplyToCollectionWithExistingSortStage() + { + $aggregationBuilderProphecy = $this->prophesize(Builder::class); + + $aggregationBuilderProphecy->sort(['name' => 'asc'])->shouldNotBeCalled(); + $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willReturn(new Sort($aggregationBuilder = $aggregationBuilderProphecy->reveal(), 'field')); + + $managerRegistryProphecy = $this->prophesize(ManagerRegistry::class); + $managerRegistryProphecy->getManagerForClass(Dummy::class)->shouldNotBeCalled(); + + $orderExtensionTest = new OrderExtension('asc', null, $managerRegistryProphecy->reveal()); + $orderExtensionTest->applyToCollection($aggregationBuilder, Dummy::class); + } } diff --git a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php index 8b938800cfb..c14bb73131d 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php @@ -22,6 +22,7 @@ use ApiPlatform\Core\Tests\ProphecyTrait; use Doctrine\ORM\EntityManager; use Doctrine\ORM\Mapping\ClassMetadata; +use Doctrine\ORM\Query\Expr\OrderBy; use Doctrine\ORM\QueryBuilder; use PHPUnit\Framework\TestCase; @@ -166,4 +167,17 @@ public function testApplyToCollectionWithOrderOverriddenWithEmbeddedAssociation( $orderExtensionTest = new OrderExtension('asc', $resourceMetadataFactoryProphecy->reveal()); $orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), EmbeddedDummy::class); } + + public function testApplyToCollectionWithExistingOrderByDql() + { + $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([new OrderBy('o.name')]); + $queryBuilderProphecy->getEntityManager()->shouldNotBeCalled(); + $queryBuilderProphecy->getRootAliases()->shouldNotBeCalled(); + + $queryBuilder = $queryBuilderProphecy->reveal(); + $orderExtensionTest = new OrderExtension(); + $orderExtensionTest->applyToCollection($queryBuilder, new QueryNameGenerator(), Dummy::class); + } } From c048cd5ab1a72a08daa95a10b481dd770b9be5dd Mon Sep 17 00:00:00 2001 From: natepage Date: Fri, 8 Oct 2021 01:34:01 +1100 Subject: [PATCH 2/4] fix(doctrine): add missing prophecy call for orm queryBuilder in tests --- tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php index c14bb73131d..fd9d6304f6a 100644 --- a/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/Orm/Extension/OrderExtensionTest.php @@ -37,6 +37,7 @@ public function testApplyToCollectionWithValidOrder() { $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->addOrderBy('o.name', 'asc')->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -57,6 +58,7 @@ public function testApplyToCollectionWithWrongOrder() { $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->addOrderBy('o.name', 'asc')->shouldNotBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -78,6 +80,7 @@ public function testApplyToCollectionWithOrderOverridden() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->addOrderBy('o.foo', 'DESC')->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -101,6 +104,7 @@ public function testApplyToCollectionWithOrderOverriddenWithNoDirection() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->addOrderBy('o.foo', 'ASC')->shouldBeCalled(); $queryBuilderProphecy->addOrderBy('o.bar', 'DESC')->shouldBeCalled(); @@ -125,6 +129,7 @@ public function testApplyToCollectionWithOrderOverriddenWithAssociation() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->getDQLPart('join')->willReturn(['o' => []])->shouldBeCalled(); $queryBuilderProphecy->innerJoin('o.author', 'author_a1', null, null)->shouldBeCalled(); $queryBuilderProphecy->addOrderBy('author_a1.name', 'ASC')->shouldBeCalled(); @@ -149,6 +154,7 @@ public function testApplyToCollectionWithOrderOverriddenWithEmbeddedAssociation( { $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $queryBuilderProphecy = $this->prophesize(QueryBuilder::class); + $queryBuilderProphecy->getDQLPart('orderBy')->shouldBeCalled()->willReturn([]); $queryBuilderProphecy->getRootAliases()->willReturn(['o']); $queryBuilderProphecy->addOrderBy('o.embeddedDummy.dummyName', 'DESC')->shouldBeCalled(); From a720793b7a3515ca37e402d17bb0ba2d0a008d0c Mon Sep 17 00:00:00 2001 From: natepage Date: Fri, 8 Oct 2021 01:39:14 +1100 Subject: [PATCH 3/4] fix(phpcs): fix cs --- src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php | 2 +- .../Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php index 71fcd2af2e5..17f6a32ccbe 100644 --- a/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php +++ b/src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php @@ -115,7 +115,7 @@ private function hasSortStage(Builder $aggregationBuilder): bool $shouldStop = true; } - $index++; + ++$index; } while (!$shouldStop); // No stage was sort, and we iterated through all stages diff --git a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php index 639e2404fd8..8d0343ea384 100644 --- a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php @@ -24,8 +24,8 @@ use Doctrine\ODM\MongoDB\DocumentManager; use Doctrine\ODM\MongoDB\Mapping\ClassMetadata; use Doctrine\Persistence\ManagerRegistry; +use OutOfRangeException; use PHPUnit\Framework\TestCase; -use \OutOfRangeException; /** * @group mongodb From 5a00b29bb77920880ad6e2231982982cee58d9c4 Mon Sep 17 00:00:00 2001 From: natepage Date: Mon, 11 Oct 2021 11:08:39 +1100 Subject: [PATCH 4/4] fix(tests): remove shouldBeCalled on aggregationBuilder willThrow OutOfRangeException --- .../MongoDbOdm/Extension/OrderExtensionTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php index 8d0343ea384..5c5b8466a21 100644 --- a/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php +++ b/tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php @@ -40,7 +40,7 @@ public function testApplyToCollectionWithValidOrder() { $aggregationBuilderProphecy = $this->prophesize(Builder::class); - $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); + $aggregationBuilderProphecy->getStage(0)->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['name' => 'asc'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -61,7 +61,7 @@ public function testApplyToCollectionWithWrongOrder() { $aggregationBuilderProphecy = $this->prophesize(Builder::class); - $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); + $aggregationBuilderProphecy->getStage(0)->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['name' => 'asc'])->shouldNotBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -83,7 +83,7 @@ public function testApplyToCollectionWithOrderOverridden() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $aggregationBuilderProphecy = $this->prophesize(Builder::class); - $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); + $aggregationBuilderProphecy->getStage(0)->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['foo' => 'DESC'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class); @@ -107,7 +107,7 @@ public function testApplyToCollectionWithOrderOverriddenWithNoDirection() $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class); $aggregationBuilderProphecy = $this->prophesize(Builder::class); - $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); + $aggregationBuilderProphecy->getStage(0)->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['foo' => 'ASC'])->shouldBeCalled(); $aggregationBuilderProphecy->sort(['foo' => 'ASC', 'bar' => 'DESC'])->shouldBeCalled(); @@ -138,7 +138,7 @@ public function testApplyToCollectionWithOrderOverriddenWithAssociation() $lookupProphecy->alias('author_lkup')->shouldBeCalled(); $aggregationBuilderProphecy->lookup(Dummy::class)->shouldBeCalled()->willReturn($lookupProphecy->reveal()); $aggregationBuilderProphecy->unwind('$author_lkup')->shouldBeCalled(); - $aggregationBuilderProphecy->getStage(0)->shouldBeCalled()->willThrow(new OutOfRangeException('message')); + $aggregationBuilderProphecy->getStage(0)->willThrow(new OutOfRangeException('message')); $aggregationBuilderProphecy->sort(['author_lkup.name' => 'ASC'])->shouldBeCalled(); $classMetadataProphecy = $this->prophesize(ClassMetadata::class);