Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1267 - order by broken in pagination logic (reverts #1220) #1283

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 27 additions & 95 deletions lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@

namespace Doctrine\ORM\Tools\Pagination;

use Doctrine\DBAL\Platforms\MySqlPlatform;
use Doctrine\DBAL\Platforms\OraclePlatform;
use Doctrine\DBAL\Platforms\SQLServerPlatform;
use Doctrine\ORM\Query\AST\OrderByClause;
use Doctrine\ORM\Query\AST\PathExpression;
use Doctrine\ORM\Query\SqlWalker;
use Doctrine\ORM\Query\AST\SelectStatement;
Expand Down Expand Up @@ -60,18 +56,6 @@ class LimitSubqueryOutputWalker extends SqlWalker
*/
private $maxResults;

/**
* @var \Doctrine\ORM\EntityManager
*/
private $em;

/**
* The quote strategy.
*
* @var \Doctrine\ORM\Mapping\QuoteStrategy
*/
private $quoteStrategy;

/**
* Constructor.
*
Expand All @@ -94,9 +78,6 @@ public function __construct($query, $parserResult, array $queryComponents)
$this->maxResults = $query->getMaxResults();
$query->setFirstResult(null)->setMaxResults(null);

$this->em = $query->getEntityManager();
$this->quoteStrategy = $this->em->getConfiguration()->getQuoteStrategy();

parent::__construct($query, $parserResult, $queryComponents);
}

Expand All @@ -111,11 +92,6 @@ public function __construct($query, $parserResult, array $queryComponents)
*/
public function walkSelectStatement(SelectStatement $AST)
{
// Remove order by clause from the inner query
// It will be re-appended in the outer select generated by this method
$orderByClause = $AST->orderByClause;
$AST->orderByClause = null;

// Set every select expression as visible(hidden = false) to
// make $AST have scalar mappings properly - this is relevant for referencing selected
// fields from outside the subquery, for example in the ORDER BY segment
Expand Down Expand Up @@ -187,7 +163,7 @@ public function walkSelectStatement(SelectStatement $AST)
implode(', ', $sqlIdentifier), $innerSql);

// http://www.doctrine-project.org/jira/browse/DDC-1958
$sql = $this->preserveSqlOrdering($sqlIdentifier, $innerSql, $sql, $orderByClause);
$sql = $this->preserveSqlOrdering($AST, $sqlIdentifier, $innerSql, $sql);

// Apply the limit and offset.
$sql = $this->platform->modifyLimitQuery(
Expand All @@ -206,29 +182,41 @@ public function walkSelectStatement(SelectStatement $AST)
}

/**
* Generates new SQL for statements with an order by clause
* Generates new SQL for Postgresql or Oracle if necessary.
*
* @param SelectStatement $AST
* @param array $sqlIdentifier
* @param string $innerSql
* @param string $sql
* @param OrderByClause $orderByClause
*
* @return string
* @return void
*/
public function preserveSqlOrdering(array $sqlIdentifier, $innerSql, $sql, $orderByClause)
public function preserveSqlOrdering(SelectStatement $AST, array $sqlIdentifier, $innerSql, $sql)
{
// If the sql statement has an order by clause, we need to wrap it in a new select distinct
// statement
if ($orderByClause instanceof OrderByClause) {
// Rebuild the order by clause to work in the scope of the new select statement
/** @var array $sqlOrderColumns an array of items that need to be included in the select list */
/** @var array $orderBy an array of rebuilt order by items */
list($sqlOrderColumns, $orderBy) = $this->rebuildOrderByClauseForOuterScope($orderByClause);

// Identifiers are always included in the select list, so there's no need to include them twice
// For every order by, find out the SQL alias by inspecting the ResultSetMapping.
$sqlOrderColumns = array();
$orderBy = array();
if (isset($AST->orderByClause)) {
foreach ($AST->orderByClause->orderByItems as $item) {
$expression = $item->expression;

$possibleAliases = $expression instanceof PathExpression
? array_keys($this->rsm->fieldMappings, $expression->field)
: array_keys($this->rsm->scalarMappings, $expression);

foreach ($possibleAliases as $alias) {
if (!is_object($expression) || $this->rsm->columnOwnerMap[$alias] == $expression->identificationVariable) {
$sqlOrderColumns[] = $alias;
$orderBy[] = $alias . ' ' . $item->type;
break;
}
}
}
// remove identifier aliases
$sqlOrderColumns = array_diff($sqlOrderColumns, $sqlIdentifier);
}

// Build the select distinct statement
if (count($orderBy)) {
$sql = sprintf(
'SELECT DISTINCT %s FROM (%s) dctrn_result ORDER BY %s',
implode(', ', array_merge($sqlIdentifier, $sqlOrderColumns)),
Expand All @@ -239,60 +227,4 @@ public function preserveSqlOrdering(array $sqlIdentifier, $innerSql, $sql, $orde

return $sql;
}

/**
* Generates a new order by clause that works in the scope of a select query wrapping the original
*
* @param OrderByClause $orderByClause
* @return array
*/
protected function rebuildOrderByClauseForOuterScope(OrderByClause $orderByClause) {
$dqlAliasToSqlTableAliasMap
= $searchPatterns
= $replacements
= $dqlAliasToClassMap
= $selectListAdditions
= $orderByItems
= array();

// Generate DQL alias -> SQL table alias mapping
foreach(array_keys($this->rsm->aliasMap) as $dqlAlias) {
$dqlAliasToClassMap[$dqlAlias] = $class = $this->queryComponents[$dqlAlias]['metadata'];
$dqlAliasToSqlTableAliasMap[$dqlAlias] = $this->getSQLTableAlias($class->getTableName(), $dqlAlias);
}

// Pattern to find table path expressions in the order by clause
$fieldSearchPattern = "/(?<![a-z0-9_])%s\.%s(?![a-z0-9_])/i";

// Generate search patterns for each field's path expression in the order by clause
foreach($this->rsm->fieldMappings as $fieldAlias => $columnName) {
$dqlAliasForFieldAlias = $this->rsm->columnOwnerMap[$fieldAlias];
$columnName = $this->quoteStrategy->getColumnName(
$columnName,
$dqlAliasToClassMap[$dqlAliasForFieldAlias],
$this->em->getConnection()->getDatabasePlatform()
);

$sqlTableAliasForFieldAlias = $dqlAliasToSqlTableAliasMap[$dqlAliasForFieldAlias];

$searchPatterns[] = sprintf($fieldSearchPattern, $sqlTableAliasForFieldAlias, $columnName);
$replacements[] = $fieldAlias;
}

foreach($orderByClause->orderByItems as $orderByItem) {
// Walk order by item to get string representation of it
$orderByItem = $this->walkOrderByItem($orderByItem);

// Replace path expressions in the order by clause with their column alias
$orderByItem = preg_replace($searchPatterns, $replacements, $orderByItem);

// The order by items are not required to be in the select list on Oracle and PostgreSQL, but
// for the sake of simplicity, order by items will be included in the select list on all platforms.
// This doesn't impact functionality.
$selectListAdditions[] = trim(preg_replace('/([^ ]+) (?:asc|desc)/i', '$1', $orderByItem));
$orderByItems[] = $orderByItem;
}

return array($selectListAdditions, $orderByItems);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
use Doctrine\DBAL\Platforms\MySqlPlatform;
use Doctrine\DBAL\Platforms\OraclePlatform;
use Doctrine\DBAL\Platforms\PostgreSqlPlatform;
use Doctrine\DBAL\Platforms\SQLServerPlatform;
use Doctrine\ORM\Query;

class LimitSubqueryOutputWalkerTest extends PaginationTestCase
Expand Down Expand Up @@ -34,7 +33,7 @@ public function testLimitSubqueryWithSortPg()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT id_0, title_1 FROM (SELECT m0_.id AS id_0, m0_.title AS title_1, c1_.id AS id_2, a2_.id AS id_3, a2_.name AS name_4, m0_.author_id AS author_id_5, m0_.category_id AS category_id_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id) dctrn_result ORDER BY title_1 ASC", $limitQuery->getSql()
"SELECT DISTINCT id_0, title_1 FROM (SELECT m0_.id AS id_0, m0_.title AS title_1, c1_.id AS id_2, a2_.id AS id_3, a2_.name AS name_4, m0_.author_id AS author_id_5, m0_.category_id AS category_id_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id ORDER BY m0_.title ASC) dctrn_result ORDER BY title_1 ASC", $limitQuery->getSql()
);

$this->entityManager->getConnection()->setDatabasePlatform($odp);
Expand All @@ -52,7 +51,7 @@ public function testLimitSubqueryWithScalarSortPg()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id) dctrn_result ORDER BY sclr_0 ASC",
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id ORDER BY sclr_0 ASC) dctrn_result ORDER BY sclr_0 ASC",
$limitQuery->getSql()
);

Expand All @@ -71,7 +70,7 @@ public function testLimitSubqueryWithMixedSortPg()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id) dctrn_result ORDER BY sclr_0 ASC, id_1 DESC",
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id ORDER BY sclr_0 ASC, u1_.id DESC) dctrn_result ORDER BY sclr_0 ASC, id_1 DESC",
$limitQuery->getSql()
);

Expand All @@ -90,7 +89,7 @@ public function testLimitSubqueryWithHiddenScalarSortPg()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id) dctrn_result ORDER BY sclr_0 ASC, id_1 DESC",
"SELECT DISTINCT id_1, sclr_0 FROM (SELECT COUNT(g0_.id) AS sclr_0, u1_.id AS id_1, g0_.id AS id_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id ORDER BY sclr_0 ASC, u1_.id DESC) dctrn_result ORDER BY sclr_0 ASC, id_1 DESC",
$limitQuery->getSql()
);

Expand Down Expand Up @@ -119,7 +118,7 @@ public function testLimitSubqueryWithSortOracle()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT ID_0, TITLE_1 FROM (SELECT m0_.id AS ID_0, m0_.title AS TITLE_1, c1_.id AS ID_2, a2_.id AS ID_3, a2_.name AS NAME_4, m0_.author_id AS AUTHOR_ID_5, m0_.category_id AS CATEGORY_ID_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id) dctrn_result ORDER BY TITLE_1 ASC", $limitQuery->getSql()
"SELECT DISTINCT ID_0, TITLE_1 FROM (SELECT m0_.id AS ID_0, m0_.title AS TITLE_1, c1_.id AS ID_2, a2_.id AS ID_3, a2_.name AS NAME_4, m0_.author_id AS AUTHOR_ID_5, m0_.category_id AS CATEGORY_ID_6 FROM MyBlogPost m0_ INNER JOIN Category c1_ ON m0_.category_id = c1_.id INNER JOIN Author a2_ ON m0_.author_id = a2_.id ORDER BY m0_.title ASC) dctrn_result ORDER BY TITLE_1 ASC", $limitQuery->getSql()
);

$this->entityManager->getConnection()->setDatabasePlatform($odp);
Expand All @@ -138,7 +137,7 @@ public function testLimitSubqueryWithScalarSortOracle()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT ID_1, SCLR_0 FROM (SELECT COUNT(g0_.id) AS SCLR_0, u1_.id AS ID_1, g0_.id AS ID_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id) dctrn_result ORDER BY SCLR_0 ASC",
"SELECT DISTINCT ID_1, SCLR_0 FROM (SELECT COUNT(g0_.id) AS SCLR_0, u1_.id AS ID_1, g0_.id AS ID_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id ORDER BY SCLR_0 ASC) dctrn_result ORDER BY SCLR_0 ASC",
$limitQuery->getSql()
);

Expand All @@ -158,7 +157,7 @@ public function testLimitSubqueryWithMixedSortOracle()
$limitQuery->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
"SELECT DISTINCT ID_1, SCLR_0 FROM (SELECT COUNT(g0_.id) AS SCLR_0, u1_.id AS ID_1, g0_.id AS ID_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id) dctrn_result ORDER BY SCLR_0 ASC, ID_1 DESC",
"SELECT DISTINCT ID_1, SCLR_0 FROM (SELECT COUNT(g0_.id) AS SCLR_0, u1_.id AS ID_1, g0_.id AS ID_2 FROM User u1_ INNER JOIN user_group u2_ ON u1_.id = u2_.user_id INNER JOIN groups g0_ ON g0_.id = u2_.group_id ORDER BY SCLR_0 ASC, u1_.id DESC) dctrn_result ORDER BY SCLR_0 ASC, ID_1 DESC",
$limitQuery->getSql()
);

Expand Down Expand Up @@ -208,37 +207,7 @@ public function testCountQueryWithArithmeticOrderByCondition()
$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertSame(
'SELECT DISTINCT id_0, (1 - 1000) * 1 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1 FROM Author a0_) dctrn_result ORDER BY (1 - 1000) * 1 DESC',
$query->getSQL()
);
}

public function testCountQueryWithComplexScalarOrderByItem()
{
$query = $this->entityManager->createQuery(
'SELECT a FROM Doctrine\Tests\ORM\Tools\Pagination\Avatar a ORDER BY a.image_height * a.image_width DESC'
);
$this->entityManager->getConnection()->setDatabasePlatform(new MySqlPlatform());

$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertSame(
'SELECT DISTINCT id_0, image_height_2 * image_width_3 FROM (SELECT a0_.id AS id_0, a0_.image AS image_1, a0_.image_height AS image_height_2, a0_.image_width AS image_width_3, a0_.image_alt_desc AS image_alt_desc_4, a0_.user_id AS user_id_5 FROM Avatar a0_) dctrn_result ORDER BY image_height_2 * image_width_3 DESC',
$query->getSQL()
);
}

public function testCountQueryWithComplexScalarOrderByItemOracle()
{
$query = $this->entityManager->createQuery(
'SELECT a FROM Doctrine\Tests\ORM\Tools\Pagination\Avatar a ORDER BY a.image_height * a.image_width DESC'
);
$this->entityManager->getConnection()->setDatabasePlatform(new OraclePlatform());

$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertSame(
'SELECT DISTINCT ID_0, IMAGE_HEIGHT_2 * IMAGE_WIDTH_3 FROM (SELECT a0_.id AS ID_0, a0_.image AS IMAGE_1, a0_.image_height AS IMAGE_HEIGHT_2, a0_.image_width AS IMAGE_WIDTH_3, a0_.image_alt_desc AS IMAGE_ALT_DESC_4, a0_.user_id AS USER_ID_5 FROM Avatar a0_) dctrn_result ORDER BY IMAGE_HEIGHT_2 * IMAGE_WIDTH_3 DESC',
'SELECT DISTINCT id_0 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1 FROM Author a0_ ORDER BY (1 - 1000) * 1 DESC) dctrn_result',
$query->getSQL()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeroedin-bill what should I do about these tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert them for now- leaving on vacation today, returning Feb. 1- I'll
devise a fix then. Would appreciate some guidance per my comment in the
failing testcase PR thread. Ciao--
On Jan 24, 2015 7:15 AM, "Marco Pivetta" [email protected] wrote:

In
tests/Doctrine/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php
#1283 (comment):

@@ -208,37 +207,7 @@ public function testCountQueryWithArithmeticOrderByCondition()
$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

     $this->assertSame(
  •        'SELECT DISTINCT id_0, (1 - 1000) \* 1 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1 FROM Author a0_) dctrn_result ORDER BY (1 - 1000) \* 1 DESC',
    

@zeroedin-bill https://github.com/zeroedin-bill what should I do about
these tests?


Reply to this email directly or view it on GitHub
https://github.com/doctrine/doctrine2/pull/1283/files#r23496003.

);
}
Expand All @@ -255,22 +224,21 @@ public function testLimitSubqueryWithHiddenSelectionInOrderBy()
$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertEquals(
'SELECT DISTINCT id_0, name_2 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1, a0_.name AS name_2 FROM Author a0_) dctrn_result ORDER BY name_2 DESC',
'SELECT DISTINCT id_0, name_2 FROM (SELECT a0_.id AS id_0, a0_.name AS name_1, a0_.name AS name_2 FROM Author a0_ ORDER BY name_2 DESC) dctrn_result ORDER BY name_2 DESC',
$query->getSql()
);
}

public function testLimitSubqueryWithColumnWithSortDirectionInName()
public function testLimitSubqueryWithOrderByInnerJoined()
{
$query = $this->entityManager->createQuery(
'SELECT a FROM Doctrine\Tests\ORM\Tools\Pagination\Avatar a ORDER BY a.image_alt_desc DESC'
'SELECT b FROM Doctrine\Tests\ORM\Tools\Pagination\BlogPost b JOIN b.author a ORDER BY a.name ASC'
);
$this->entityManager->getConnection()->setDatabasePlatform(new MySqlPlatform());

$query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, 'Doctrine\ORM\Tools\Pagination\LimitSubqueryOutputWalker');

$this->assertSame(
'SELECT DISTINCT id_0, image_alt_desc_4 FROM (SELECT a0_.id AS id_0, a0_.image AS image_1, a0_.image_height AS image_height_2, a0_.image_width AS image_width_3, a0_.image_alt_desc AS image_alt_desc_4, a0_.user_id AS user_id_5 FROM Avatar a0_) dctrn_result ORDER BY image_alt_desc_4 DESC',
$this->assertEquals(
'SELECT DISTINCT id_0 FROM (SELECT b0_.id AS id_0, b0_.author_id AS author_id_1, b0_.category_id AS category_id_2 FROM BlogPost b0_ INNER JOIN Author a1_ ON b0_.author_id = a1_.id ORDER BY a1_.name ASC) dctrn_result',
$query->getSQL()
);
}
Expand Down
20 changes: 0 additions & 20 deletions tests/Doctrine/Tests/ORM/Tools/Pagination/PaginationTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,23 +145,3 @@ class User
*/
public $groups;
}

/** @Entity */
class Avatar
{
/** @Id @column(type="integer") @generatedValue */
public $id;
/**
* @OneToOne(targetEntity="User", inversedBy="avatar")
* @JoinColumn(name="user_id", referencedColumnName="id")
*/
public $user;
/** @column(type="string", length=255) */
public $image;
/** @column(type="integer") */
public $image_height;
/** @column(type="integer") */
public $image_width;
/** @column(type="string", length=255) */
public $image_alt_desc;
}