From 902ecad3af00c3e845197506adfbc090f6939a2f Mon Sep 17 00:00:00 2001 From: Vova Soroka Date: Sat, 14 Jan 2017 01:23:05 +0200 Subject: [PATCH] BAP-8103: Report/Segment builder add unexpected Order by to final query (#6547) --- .../Sorter/AbstractSorterExtension.php | 18 +- .../Extension/Sorter/OrmSorterExtension.php | 40 ++++ .../Sorter/OrmSorterExtensionTest.php | 173 +++++++++++++++++- .../Sorter/SearchSorterExtensionTest.php | 56 +++++- 4 files changed, 259 insertions(+), 28 deletions(-) diff --git a/src/Oro/Bundle/DataGridBundle/Extension/Sorter/AbstractSorterExtension.php b/src/Oro/Bundle/DataGridBundle/Extension/Sorter/AbstractSorterExtension.php index 731b76d7226..36436440c2b 100644 --- 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 index 1d46bbaf7a1..a53f3dfdee8 100644 --- 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 index da6aeb7be5d..73e89651425 100644 --- 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/src/Oro/Bundle/SearchBundle/Tests/Unit/Datagrid/Extension/Sorter/SearchSorterExtensionTest.php b/src/Oro/Bundle/SearchBundle/Tests/Unit/Datagrid/Extension/Sorter/SearchSorterExtensionTest.php index 257c2cb15fe..c3ab4a4544e 100644 --- a/src/Oro/Bundle/SearchBundle/Tests/Unit/Datagrid/Extension/Sorter/SearchSorterExtensionTest.php +++ b/src/Oro/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 => [