fix(doctrine): do not apply order extension if sorting already defined (api-platform#4493)

* Do not apply order extension if sorting already applied

* fix(doctrine): add missing prophecy call for orm queryBuilder in tests

* fix(phpcs): fix cs

* fix(tests): remove shouldBeCalled on aggregationBuilder willThrow OutOfRangeException

Author: natepage

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;
+    }
 }

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);

tests/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtensionTest.php

@@ -20,9 +20,11 @@
 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 OutOfRangeException;
 use PHPUnit\Framework\TestCase;
 
 /**
@@ -38,6 +40,7 @@ public function testApplyToCollectionWithValidOrder()
     {
         $aggregationBuilderProphecy = $this->prophesize(Builder::class);
 
+        $aggregationBuilderProphecy->getStage(0)->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)->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)->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)->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)->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);
+    }
 }

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;
 
@@ -36,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);
@@ -56,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);
@@ -77,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);
@@ -100,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();
 
@@ -124,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();
@@ -148,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();
 
@@ -166,4 +173,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);
+    }
 }