BAP-8103: Report/Segment builder add unexpected Order by to final que…

…ry (#6547)
oroinc · Jan 13, 2017 · 902ecad · 902ecad
1 parent 62371cf
commit 902ecad
Show file tree

Hide file tree

Showing 4 changed files with 259 additions and 28 deletions.
diff --git a/src/Oro/Bundle/DataGridBundle/Extension/Sorter/AbstractSorterExtension.php b/src/Oro/Bundle/DataGridBundle/Extension/Sorter/AbstractSorterExtension.php
@@ -292,6 +292,7 @@ protected function getSortersState(DatagridConfiguration $config, MetadataObject
      */
     private function getSortBy($readParameters, array $defaultSorters, array $sorters, $disableDefaultSorting)
     {
+        $sortBy = [];
         if ($readParameters) {
             $sortBy = (array)$this->getParameters()->get(self::SORTERS_ROOT_PARAM);
             $sortBy = array_filter(
@@ -301,23 +302,10 @@ function ($sortColumn) use ($sorters) {
                 },
                 ARRAY_FILTER_USE_KEY
             );
-            if (!$sortBy) {
-                $sortBy = $defaultSorters;
-            }
-        } else {
-            $sortBy = $defaultSorters;
         }
 
-        // when disable sorting option is set up, do not use any sorters
-        if ($sortBy === $defaultSorters && $disableDefaultSorting) {
-            return [];
-        }
-
-        // if default sorter was not specified, just take first sortable column
-        if (!$sortBy && $sorters) {
-            $names           = array_keys($sorters);
-            $firstSorterName = reset($names);
-            $sortBy          = [$firstSorterName => self::DIRECTION_ASC];
+        if (empty($sortBy) && !$disableDefaultSorting) {
+            $sortBy = $defaultSorters;
         }
 
         return $sortBy;

diff --git a/src/Oro/Bundle/DataGridBundle/Extension/Sorter/OrmSorterExtension.php b/src/Oro/Bundle/DataGridBundle/Extension/Sorter/OrmSorterExtension.php
@@ -2,6 +2,8 @@
 
 namespace Oro\Bundle\DataGridBundle\Extension\Sorter;
 
+use Doctrine\ORM\Query\Expr;
+
 use Oro\Bundle\DataGridBundle\Datagrid\Common\DatagridConfiguration;
 use Oro\Bundle\DataGridBundle\Datasource\DatasourceInterface;
 use Oro\Bundle\DataGridBundle\Datasource\Orm\OrmDatasource;
@@ -26,4 +28,42 @@ protected function addSorterToDatasource(array $sorter, $direction, DatasourceIn
         /* @var OrmDatasource $datasource */
         $datasource->getQueryBuilder()->addOrderBy($sorter['data_name'], $direction);
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function visitDatasource(DatagridConfiguration $config, DatasourceInterface $datasource)
+    {
+        parent::visitDatasource($config, $datasource);
+
+        // ensure that ORDER BY is specified explicitly, in case if sorting was not requested
+        // use sorting by
+        // - the primary key of the root table if the query does not have GROUP BY
+        // - the first column of GROUP BY if this clause exists
+        // if ORDER BY is not given, the order of rows is not predictable and they are returned
+        // in whatever order SQL server finds fastest to produce.
+        /** @var OrmDatasource $datasource */
+        $qb = $datasource->getQueryBuilder();
+        $orderBy = $qb->getDQLPart('orderBy');
+        if (empty($orderBy)) {
+            $groupBy = $qb->getDQLPart('groupBy');
+            if (empty($groupBy)) {
+                $rootEntities = $qb->getRootEntities();
+                $rootEntity = reset($rootEntities);
+                if ($rootEntity) {
+                    $rootAliases = $qb->getRootAliases();
+                    $rootAlias = reset($rootAliases);
+                    $rootIdFieldNames = $qb->getEntityManager()
+                        ->getClassMetadata($rootEntity)
+                        ->getIdentifierFieldNames();
+                    $qb->addOrderBy($rootAlias . '.' . reset($rootIdFieldNames));
+                }
+            } else {
+                /** @var Expr\GroupBy $firstExpr */
+                $firstExpr = reset($groupBy);
+                $exprParts = $firstExpr->getParts();
+                $qb->addOrderBy(reset($exprParts));
+            }
+        }
+    }
 }
diff --git a/src/Oro/Bundle/DataGridBundle/Tests/Unit/Extension/Sorter/OrmSorterExtensionTest.php b/src/Oro/Bundle/DataGridBundle/Tests/Unit/Extension/Sorter/OrmSorterExtensionTest.php
@@ -2,7 +2,11 @@
 
 namespace Oro\Bundle\DataGridBundle\Tests\Unit\Extension\Sorter;
 
+use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\ORM\QueryBuilder;
 use Oro\Bundle\DataGridBundle\Datagrid\ParameterBag;
+use Oro\Bundle\DataGridBundle\Datasource\Orm\OrmDatasource;
 use Oro\Bundle\DataGridBundle\Extension\Sorter\OrmSorterExtension;
 use Oro\Bundle\DataGridBundle\Datagrid\Common\DatagridConfiguration;
 use Oro\Bundle\DataGridBundle\Datagrid\Common\MetadataObject;
@@ -125,8 +129,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => false,
                         ],
                     ],
-                    'initialState' => ['sorters' => ['name' => 'ASC',]],
-                    'state' => ['sorters' => ['name' => 'ASC',]],
+                    'initialState' => ['sorters' => []],
+                    'state' => ['sorters' => []],
                 ]
             ],
             'multiple' => [
@@ -154,8 +158,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => false,
                         ],
                     ],
-                    'initialState' => ['sorters' => ['name' => 'ASC',]],
-                    'state' => ['sorters' => ['name' => 'ASC',]],
+                    'initialState' => ['sorters' => []],
+                    'state' => ['sorters' => []],
                 ]
             ],
             'toolbar' => [
@@ -190,8 +194,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => true,
                         ],
                     ],
-                    'initialState' => ['sorters' => ['name' => 'ASC',]],
-                    'state' => ['sorters' => ['name' => 'ASC',]],
+                    'initialState' => ['sorters' => []],
+                    'state' => ['sorters' => []],
                 ]
             ]
         ];
@@ -247,4 +251,161 @@ public function visitMetadataUnknownColumnDataProvider()
             ],
         ];
     }
+
+    public function testVisitDatasourceWithoutDefaultSorting()
+    {
+        $em = $this->getMockBuilder(EntityManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $metadata = $this->getMockBuilder(ClassMetadata::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $em->expects(self::once())
+            ->method('getClassMetadata')
+            ->with('Test\Entity')
+            ->willReturn($metadata);
+        $metadata->expects(self::once())
+            ->method('getIdentifierFieldNames')
+            ->willReturn(['id']);
+
+        $qb = new QueryBuilder($em);
+        $qb->select('e.id')->from('Test\Entity', 'e');
+
+        $datasource = $this->getMockBuilder(OrmDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $datasource->expects(self::once())
+            ->method('getQueryBuilder')
+            ->willReturn($qb);
+
+        $this->extension->setParameters(new ParameterBag());
+        $this->extension->visitDatasource(
+            DatagridConfiguration::create(['sorters' => ['columns' => []]]),
+            $datasource
+        );
+
+        self::assertEquals(
+            'SELECT e.id FROM Test\Entity e ORDER BY e.id ASC',
+            $qb->getDQL()
+        );
+    }
+
+    public function testVisitDatasourceWhenQueryAlreadyHasOrderBy()
+    {
+        $em = $this->getMockBuilder(EntityManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $em->expects(self::never())
+            ->method('getClassMetadata');
+
+        $qb = new QueryBuilder($em);
+        $qb->select('e.id')->from('Test\Entity', 'e')->addOrderBy('e.name');
+
+        $datasource = $this->getMockBuilder(OrmDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $datasource->expects(self::once())
+            ->method('getQueryBuilder')
+            ->willReturn($qb);
+
+        $this->extension->setParameters(new ParameterBag());
+        $this->extension->visitDatasource(
+            DatagridConfiguration::create(['sorters' => ['columns' => []]]),
+            $datasource
+        );
+
+        self::assertEquals(
+            'SELECT e.id FROM Test\Entity e ORDER BY e.name ASC',
+            $qb->getDQL()
+        );
+    }
+
+    public function testVisitDatasourceWithoutDefaultSortingForEmptyQuery()
+    {
+        $em = $this->getMockBuilder(EntityManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $em->expects(self::never())
+            ->method('getClassMetadata');
+
+        $qb = new QueryBuilder($em);
+
+        $datasource = $this->getMockBuilder(OrmDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $datasource->expects(self::once())
+            ->method('getQueryBuilder')
+            ->willReturn($qb);
+
+        $this->extension->setParameters(new ParameterBag());
+        $this->extension->visitDatasource(
+            DatagridConfiguration::create(['sorters' => ['columns' => []]]),
+            $datasource
+        );
+
+        self::assertEquals(
+            [],
+            $qb->getDQLPart('orderBy')
+        );
+    }
+
+    public function testVisitDatasourceWithoutDefaultSortingAndGroupBy()
+    {
+        $em = $this->getMockBuilder(EntityManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $em->expects(self::never())
+            ->method('getClassMetadata');
+
+        $qb = new QueryBuilder($em);
+        $qb->select('e.id')->from('Test\Entity', 'e')->groupBy('e.name');
+
+        $datasource = $this->getMockBuilder(OrmDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $datasource->expects(self::once())
+            ->method('getQueryBuilder')
+            ->willReturn($qb);
+
+        $this->extension->setParameters(new ParameterBag());
+        $this->extension->visitDatasource(
+            DatagridConfiguration::create(['sorters' => ['columns' => []]]),
+            $datasource
+        );
+
+        self::assertEquals(
+            'SELECT e.id FROM Test\Entity e GROUP BY e.name ORDER BY e.name ASC',
+            $qb->getDQL()
+        );
+    }
+
+    public function testVisitDatasourceWithoutDefaultSortingAndMultipleGroupBy()
+    {
+        $em = $this->getMockBuilder(EntityManager::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $em->expects(self::never())
+            ->method('getClassMetadata');
+
+        $qb = new QueryBuilder($em);
+        $qb->select('e.id')->from('Test\Entity', 'e')->addGroupBy('e.id')->addGroupBy('e.name');
+
+        $datasource = $this->getMockBuilder(OrmDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $datasource->expects(self::once())
+            ->method('getQueryBuilder')
+            ->willReturn($qb);
+
+        $this->extension->setParameters(new ParameterBag());
+        $this->extension->visitDatasource(
+            DatagridConfiguration::create(['sorters' => ['columns' => []]]),
+            $datasource
+        );
+
+        self::assertEquals(
+            'SELECT e.id FROM Test\Entity e GROUP BY e.id, e.name ORDER BY e.id ASC',
+            $qb->getDQL()
+        );
+    }
 }
diff --git a/...ro/Bundle/SearchBundle/Tests/Unit/Datagrid/Extension/Sorter/SearchSorterExtensionTest.php b/...ro/Bundle/SearchBundle/Tests/Unit/Datagrid/Extension/Sorter/SearchSorterExtensionTest.php
@@ -131,8 +131,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => false,
                         ],
                     ],
-                    'initialState'             => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
-                    'state'                    => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
+                    'initialState'             => [Configuration::SORTERS_KEY => []],
+                    'state'                    => [Configuration::SORTERS_KEY => []],
                 ]
             ],
             'multiple' => [
@@ -160,8 +160,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => false,
                         ],
                     ],
-                    'initialState'             => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
-                    'state'                    => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
+                    'initialState'             => [Configuration::SORTERS_KEY => []],
+                    'state'                    => [Configuration::SORTERS_KEY => []],
                 ]
             ],
             'toolbar'  => [
@@ -196,8 +196,8 @@ public function visitMetadataDataProvider()
                             'addSorting' => true,
                         ],
                     ],
-                    'initialState'             => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
-                    'state'                    => [Configuration::SORTERS_KEY => ['name' => 'ASC',]],
+                    'initialState'             => [Configuration::SORTERS_KEY => []],
+                    'state'                    => [Configuration::SORTERS_KEY => []],
                 ]
             ]
         ];
@@ -263,6 +263,9 @@ public function testVisitDatasourceWithValidType($configDataType)
     {
         $config = DatagridConfiguration::create([
             Configuration::SORTERS_KEY => [
+                Configuration::DEFAULT_SORTERS_KEY => [
+                    'testColumn' => 'ASC'
+                ],
                 Configuration::COLUMNS_KEY => [
                     'testColumn' => [
                         'data_name' => 'testColumn',
@@ -314,6 +317,9 @@ public function testVisitDatasourceWithInvalidType()
     {
         $config = DatagridConfiguration::create([
             Configuration::SORTERS_KEY => [
+                Configuration::DEFAULT_SORTERS_KEY => [
+                    'testColumn' => 'ASC'
+                ],
                 Configuration::COLUMNS_KEY => [
                     'testColumn' => [
                         'data_name' => 'testColumn',
@@ -335,7 +341,43 @@ public function testVisitDatasourceWithInvalidType()
         $this->sorter->visitDatasource($config, $mockDatasource);
     }
 
-    public function testVisitDatasourceWithDisableDefaultSorting()
+    public function testVisitDatasourceWithDefaultSorterAndDefaultSortingIsNotDisabled()
+    {
+        $config = DatagridConfiguration::create([
+            Configuration::SORTERS_KEY => [
+                Configuration::COLUMNS_KEY                 => [
+                    'testColumn' => [
+                        'data_name' => 'testColumn',
+                        'type'      => 'string',
+                    ]
+                ],
+                Configuration::DEFAULT_SORTERS_KEY         => [
+                    'testColumn' => 'ASC',
+                ]
+            ],
+        ]);
+
+        $mockQuery = $this->getMockBuilder(SearchQueryInterface::class)
+            ->getMock();
+
+        $mockDatasource = $this->getMockBuilder(SearchDatasource::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $mockDatasource
+            ->expects($this->once())
+            ->method('getSearchQuery')
+            ->willReturn($mockQuery);
+
+        $mockParameterBag = $this->getMockBuilder(ParameterBag::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $this->sorter->setParameters($mockParameterBag);
+
+        $this->sorter->visitDatasource($config, $mockDatasource);
+    }
+
+    public function testVisitDatasourceWithNoDefaultSorterAndDisableDefaultSorting()
     {
         $config = DatagridConfiguration::create([
             Configuration::SORTERS_KEY => [