From ca0b90c9c34cd6fc47a82ceb66aa812516c49e29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Sep 2016 10:46:59 +0200 Subject: [PATCH 1/3] Adds an order by clause analyzer to be able to include external order by columns or expressions in order by columns. --- .../Database/TDBM/InnerResultIterator.php | 6 + src/Mouf/Database/TDBM/OrderByAnalyzer.php | 129 ++++++++++++++++++ src/Mouf/Database/TDBM/TDBMService.php | 47 +++++-- tests/Mouf/Database/TDBM/Dao/TestUserDao.php | 20 +++ .../Database/TDBM/OrderByAnalyzerTest.php | 70 ++++++++++ .../Database/TDBM/TDBMDaoGeneratorTest.php | 25 ++++ 6 files changed, 289 insertions(+), 8 deletions(-) create mode 100644 src/Mouf/Database/TDBM/OrderByAnalyzer.php create mode 100644 tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php diff --git a/src/Mouf/Database/TDBM/InnerResultIterator.php b/src/Mouf/Database/TDBM/InnerResultIterator.php index 05c583b..ae8749a 100644 --- a/src/Mouf/Database/TDBM/InnerResultIterator.php +++ b/src/Mouf/Database/TDBM/InnerResultIterator.php @@ -136,6 +136,12 @@ public function next() $beansData = []; foreach ($row as $i => $value) { $columnDescriptor = $this->columnDescriptors[$i]; + + if ($columnDescriptor['tableGroup'] === null) { + // A column can have no tableGroup (if it comes from an ORDER BY expression) + continue; + } + // Let's cast the value according to its type $value = $columnDescriptor['type']->convertToPHPValue($value, $this->databasePlatform); diff --git a/src/Mouf/Database/TDBM/OrderByAnalyzer.php b/src/Mouf/Database/TDBM/OrderByAnalyzer.php new file mode 100644 index 0000000..54e5c81 --- /dev/null +++ b/src/Mouf/Database/TDBM/OrderByAnalyzer.php @@ -0,0 +1,129 @@ +cache = $cache; + $this->cachePrefix = $cachePrefix; + } + + /** + * Returns an array for each sorted "column" in the form: + * + * [ + * [ + * 'type' => 'colref', + * 'table' => null, + * 'column' => 'a', + * 'direction' => 'ASC' + * ], + * [ + * 'type' => 'expr', + * 'expr' => 'RAND()', + * 'direction' => 'DESC' + * ] + * ] + * + * @param string $orderBy + * @return array + */ + public function analyzeOrderBy(string $orderBy) : array + { + $key = $this->cachePrefix.'_order_by_analysis_'.$orderBy; + $results = $this->cache->fetch($key); + if ($results !== false) { + return $results; + } + $results = $this->analyzeOrderByNoCache($orderBy); + $this->cache->save($key, $results); + return $results; + } + + private function analyzeOrderByNoCache(string $orderBy) : array + { + $sqlParser = new PHPSQLParser(); + $sql = 'SELECT 1 FROM a ORDER BY '.$orderBy; + $parsed = $sqlParser->parse($sql, true); + + $results = []; + + for ($i = 0, $count = count($parsed['ORDER']); $i < $count; $i++) { + $orderItem = $parsed['ORDER'][$i]; + if ($orderItem['expr_type'] === 'colref') { + $parts = $orderItem['no_quotes']['parts']; + $columnName = array_pop($parts); + if (!empty($parts)) { + $tableName = array_pop($parts); + } else { + $tableName = null; + } + + $results[] = [ + 'type' => 'colref', + 'table' => $tableName, + 'column' => $columnName, + 'direction' => $orderItem['direction'] + ]; + } else { + $position = $orderItem['position']; + if ($i+1 < $count) { + $nextPosition = $parsed['ORDER'][$i+1]['position']; + $str = substr($sql, $position, $nextPosition - $position); + } else { + $str = substr($sql, $position); + } + + $str = trim($str, " \t\r\n,"); + + $results[] = [ + 'type' => 'expr', + 'expr' => $this->trimDirection($str), + 'direction' => $orderItem['direction'] + ]; + } + + + } + + return $results; + } + + /** + * Trims the ASC/DESC direction at the end of the string. + * + * @param string $sql + * @return string + */ + private function trimDirection(string $sql) : string + { + preg_match('/^(.*)(\s+(DESC|ASC|))*$/Ui', $sql, $matches); + + return $matches[1]; + } +} diff --git a/src/Mouf/Database/TDBM/TDBMService.php b/src/Mouf/Database/TDBM/TDBMService.php index 3330fbb..a784eb7 100644 --- a/src/Mouf/Database/TDBM/TDBMService.php +++ b/src/Mouf/Database/TDBM/TDBMService.php @@ -115,9 +115,9 @@ class TDBMService private $toSaveObjects; /** - * The content of the cache variable. + * A cache service to be used. * - * @var array + * @var Cache|null */ private $cache; @@ -143,6 +143,11 @@ class TDBMService */ private $logger; + /** + * @var OrderByAnalyzer + */ + private $orderByAnalyzer; + /** * @param Connection $connection The DBAL DB connection to use * @param Cache|null $cache A cache service to be used @@ -181,6 +186,7 @@ public function __construct(Connection $connection, Cache $cache = null, SchemaA $this->rootLogger = $logger; $this->setLogLevel(LogLevel::WARNING); } + $this->orderByAnalyzer = new OrderByAnalyzer($this->cache, $this->cachePrefix); } /** @@ -1315,7 +1321,7 @@ public function findObjects($mainTable, $filter = null, array $parameters = arra $parameters = array_merge($parameters, $additionalParameters); - list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, $additionalTablesFetch); + list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, $additionalTablesFetch, $orderString); $sql = 'SELECT DISTINCT '.implode(', ', $columnsList).' FROM MAGICJOIN('.$mainTable.')'; @@ -1334,7 +1340,6 @@ public function findObjects($mainTable, $filter = null, array $parameters = arra if (!empty($orderString)) { $sql .= ' ORDER BY '.$orderString; - $countSql .= ' ORDER BY '.$orderString; } if ($mode !== null && $mode !== self::MODE_CURSOR && $mode !== self::MODE_ARRAY) { @@ -1394,7 +1399,7 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu }, $columnDescList)).' FROM '.$from; if (count($allFetchedTables) > 1) { - list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, []); + list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, [], $orderString); } // Let's compute the COUNT. @@ -1458,14 +1463,17 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu /** * Returns the column list that must be fetched for the SQL request. * + * Note: MySQL dictates that ORDER BYed columns should appear in the SELECT clause. + * * @param string $mainTable * @param array $additionalTablesFetch + * @param string|null $orderBy * * @return array * * @throws \Doctrine\DBAL\Schema\SchemaException */ - private function getColumnsList(string $mainTable, array $additionalTablesFetch = array()) + private function getColumnsList(string $mainTable, array $additionalTablesFetch = array(), string $orderBy = null) { // From the table name and the additional tables we want to fetch, let's build a list of all tables // that must be part of the select columns. @@ -1477,6 +1485,31 @@ private function getColumnsList(string $mainTable, array $additionalTablesFetch $tableGroups[$table] = $tableGroupName; } + $columnsList = []; + $columnDescList = []; + $sortColumn = 0; + + // Now, let's deal with "order by columns" + if ($orderBy !== null) { + $orderByColumns = $this->orderByAnalyzer->analyzeOrderBy($orderBy); + + // If we sort by a column, there is a high chance we will fetch the bean containing this column. + // Hence, we should add the table to the $additionalTablesFetch + foreach ($orderByColumns as $orderByColumn) { + if ($orderByColumn['type'] === 'colref' && $orderByColumn['table'] !== null) { + $additionalTablesFetch[] = $orderByColumn['table']; + } elseif ($orderByColumn['type'] === 'expr') { + $sortColumnName = 'sort_column_'.$sortColumn; + $columnsList[] = $orderByColumn['expr'].' as '.$sortColumnName; + $columnDescList[] = [ + 'tableGroup' => null + ]; + $sortColumn++; + } + } + } + + foreach ($additionalTablesFetch as $additionalTable) { $relatedTables = $this->_getRelatedTablesByInheritance($additionalTable); $tableGroupName = $this->getTableGroupName($relatedTables); @@ -1489,8 +1522,6 @@ private function getColumnsList(string $mainTable, array $additionalTablesFetch // Let's remove any duplicate $allFetchedTables = array_flip(array_flip($allFetchedTables)); - $columnsList = []; - $columnDescList = []; $schema = $this->tdbmSchemaAnalyzer->getSchema(); // Now, let's build the column list diff --git a/tests/Mouf/Database/TDBM/Dao/TestUserDao.php b/tests/Mouf/Database/TDBM/Dao/TestUserDao.php index 46f9c29..b51ff05 100644 --- a/tests/Mouf/Database/TDBM/Dao/TestUserDao.php +++ b/tests/Mouf/Database/TDBM/Dao/TestUserDao.php @@ -59,4 +59,24 @@ public function getUsersWrongTableName() { return $this->find('contacts.manager_id = 1'); } + + /** + * Returns a list of users, sorted by a table on an external column. + * + * @return \Mouf\Database\TDBM\ResultIterator|\Mouf\Database\TDBM\Test\Dao\Bean\UserBean[]|\Mouf\Database\TDBM\Test\Dao\Generated\ResultArray + */ + public function getUsersByCountryName() + { + return $this->find(null, [], 'country.label DESC'); + } + + /** + * A test to sort by function. + * + * @return \Mouf\Database\TDBM\ResultIterator|\Mouf\Database\TDBM\Test\Dao\Bean\UserBean[]|\Mouf\Database\TDBM\Test\Dao\Generated\ResultArray + */ + public function getUsersByReversedCountryName() + { + return $this->find(null, [], 'REVERSE(country.label) ASC'); + } } diff --git a/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php b/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php new file mode 100644 index 0000000..37d7e0a --- /dev/null +++ b/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php @@ -0,0 +1,70 @@ +analyzeOrderBy('`a`, b desc, rand() DESC, masc, mytable.mycol'); + + $this->assertCount(5, $results); + $this->assertEquals([ + 'type' => 'colref', + 'table' => null, + 'column' => 'a', + 'direction' => 'ASC' + ], $results[0]); + $this->assertEquals([ + 'type' => 'colref', + 'table' => null, + 'column' => 'b', + 'direction' => 'DESC' + ], $results[1]); + $this->assertEquals([ + 'type' => 'expr', + 'expr' => 'rand()', + 'direction' => 'DESC' + ], $results[2]); + $this->assertEquals([ + 'type' => 'colref', + 'table' => null, + 'column' => 'masc', + 'direction' => 'ASC' + ], $results[3]); + $this->assertEquals([ + 'type' => 'colref', + 'table' => 'mytable', + 'column' => 'mycol', + 'direction' => 'ASC' + ], $results[4]); + } + + public function testExprWithAsc() + { + $analyzer = new OrderByAnalyzer(new VoidCache(), ''); + $results = $analyzer->analyzeOrderBy('foodesc + barasc'); + + $this->assertCount(1, $results); + $this->assertEquals([ + 'type' => 'expr', + 'expr' => 'foodesc + barasc', + 'direction' => 'ASC' + ], $results[0]); + } + + public function testCache() + { + $analyzer = new OrderByAnalyzer(new ArrayCache(), ''); + $results = $analyzer->analyzeOrderBy('foo'); + $results2 = $analyzer->analyzeOrderBy('foo'); + // For code coverage purpose + } + +} diff --git a/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php b/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php index 5743940..7f7962c 100644 --- a/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php +++ b/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php @@ -39,6 +39,7 @@ use Mouf\Database\TDBM\Test\Dao\RoleDao; use Mouf\Database\TDBM\Test\Dao\UserDao; use Mouf\Database\TDBM\Utils\TDBMDaoGenerator; +use PHPSQLParser\PHPSQLParser; /** */ @@ -1058,4 +1059,28 @@ public function testDisconnectedManyToOne() $this->assertCount(1, $country->getUsers()); $this->assertSame($user, $country->getUsers()[0]); } + + /** + * @depends testDaoGeneration + */ + public function testOrderByExternalCol() + { + // This test cases checks issue https://github.com/thecodingmachine/database.tdbm/issues/106 + + $userDao = new TestUserDao($this->tdbmService); + $users = $userDao->getUsersByCountryName(); + + $this->assertEquals('UK', $users[0]->getCountry()->getLabel()); + } + + /** + * @depends testDaoGeneration + */ + public function testOrderByExpression() + { + $userDao = new TestUserDao($this->tdbmService); + $users = $userDao->getUsersByReversedCountryName(); + + $this->assertEquals('Jamaica', $users[0]->getCountry()->getLabel()); + } } From eb56e6199791a6419a5e6523f39a2af77f582efd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Sep 2016 10:49:14 +0200 Subject: [PATCH 2/3] PSR2 --- src/Mouf/Database/TDBM/AbstractTDBMObject.php | 2 +- src/Mouf/Database/TDBM/OrderByAnalyzer.php | 22 +++++++++++-------- src/Mouf/Database/TDBM/TDBMService.php | 9 ++++---- .../Database/TDBM/OrderByAnalyzerTest.php | 15 +++++-------- .../Database/TDBM/TDBMDaoGeneratorTest.php | 1 - 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/Mouf/Database/TDBM/AbstractTDBMObject.php b/src/Mouf/Database/TDBM/AbstractTDBMObject.php index 608313a..9353981 100644 --- a/src/Mouf/Database/TDBM/AbstractTDBMObject.php +++ b/src/Mouf/Database/TDBM/AbstractTDBMObject.php @@ -496,7 +496,7 @@ private function removeManyToOneRelationship(string $tableName, string $foreignK * @param string $foreignKeyName * @param string $searchTableName * @param array $searchFilter - * @param string $orderString The ORDER BY part of the query. All columns must be prefixed by the table name (in the form: table.column). WARNING : This parameter is not kept when there is an additionnal or removal object ! + * @param string $orderString The ORDER BY part of the query. All columns must be prefixed by the table name (in the form: table.column). WARNING : This parameter is not kept when there is an additionnal or removal object ! * * @return AlterableResultIterator */ diff --git a/src/Mouf/Database/TDBM/OrderByAnalyzer.php b/src/Mouf/Database/TDBM/OrderByAnalyzer.php index 54e5c81..e6ad1f9 100644 --- a/src/Mouf/Database/TDBM/OrderByAnalyzer.php +++ b/src/Mouf/Database/TDBM/OrderByAnalyzer.php @@ -1,5 +1,7 @@ analyzeOrderByNoCache($orderBy); $this->cache->save($key, $results); + return $results; } @@ -73,7 +78,7 @@ private function analyzeOrderByNoCache(string $orderBy) : array $results = []; - for ($i = 0, $count = count($parsed['ORDER']); $i < $count; $i++) { + for ($i = 0, $count = count($parsed['ORDER']); $i < $count; ++$i) { $orderItem = $parsed['ORDER'][$i]; if ($orderItem['expr_type'] === 'colref') { $parts = $orderItem['no_quotes']['parts']; @@ -88,12 +93,12 @@ private function analyzeOrderByNoCache(string $orderBy) : array 'type' => 'colref', 'table' => $tableName, 'column' => $columnName, - 'direction' => $orderItem['direction'] + 'direction' => $orderItem['direction'], ]; } else { $position = $orderItem['position']; - if ($i+1 < $count) { - $nextPosition = $parsed['ORDER'][$i+1]['position']; + if ($i + 1 < $count) { + $nextPosition = $parsed['ORDER'][$i + 1]['position']; $str = substr($sql, $position, $nextPosition - $position); } else { $str = substr($sql, $position); @@ -104,11 +109,9 @@ private function analyzeOrderByNoCache(string $orderBy) : array $results[] = [ 'type' => 'expr', 'expr' => $this->trimDirection($str), - 'direction' => $orderItem['direction'] + 'direction' => $orderItem['direction'], ]; } - - } return $results; @@ -118,6 +121,7 @@ private function analyzeOrderByNoCache(string $orderBy) : array * Trims the ASC/DESC direction at the end of the string. * * @param string $sql + * * @return string */ private function trimDirection(string $sql) : string diff --git a/src/Mouf/Database/TDBM/TDBMService.php b/src/Mouf/Database/TDBM/TDBMService.php index a784eb7..9c394c4 100644 --- a/src/Mouf/Database/TDBM/TDBMService.php +++ b/src/Mouf/Database/TDBM/TDBMService.php @@ -1465,8 +1465,8 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu * * Note: MySQL dictates that ORDER BYed columns should appear in the SELECT clause. * - * @param string $mainTable - * @param array $additionalTablesFetch + * @param string $mainTable + * @param array $additionalTablesFetch * @param string|null $orderBy * * @return array @@ -1502,14 +1502,13 @@ private function getColumnsList(string $mainTable, array $additionalTablesFetch $sortColumnName = 'sort_column_'.$sortColumn; $columnsList[] = $orderByColumn['expr'].' as '.$sortColumnName; $columnDescList[] = [ - 'tableGroup' => null + 'tableGroup' => null, ]; - $sortColumn++; + ++$sortColumn; } } } - foreach ($additionalTablesFetch as $additionalTable) { $relatedTables = $this->_getRelatedTablesByInheritance($additionalTable); $tableGroupName = $this->getTableGroupName($relatedTables); diff --git a/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php b/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php index 37d7e0a..df9fd46 100644 --- a/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php +++ b/tests/Mouf/Database/TDBM/OrderByAnalyzerTest.php @@ -1,9 +1,7 @@ 'colref', 'table' => null, 'column' => 'a', - 'direction' => 'ASC' + 'direction' => 'ASC', ], $results[0]); $this->assertEquals([ 'type' => 'colref', 'table' => null, 'column' => 'b', - 'direction' => 'DESC' + 'direction' => 'DESC', ], $results[1]); $this->assertEquals([ 'type' => 'expr', 'expr' => 'rand()', - 'direction' => 'DESC' + 'direction' => 'DESC', ], $results[2]); $this->assertEquals([ 'type' => 'colref', 'table' => null, 'column' => 'masc', - 'direction' => 'ASC' + 'direction' => 'ASC', ], $results[3]); $this->assertEquals([ 'type' => 'colref', 'table' => 'mytable', 'column' => 'mycol', - 'direction' => 'ASC' + 'direction' => 'ASC', ], $results[4]); } @@ -55,7 +53,7 @@ public function testExprWithAsc() $this->assertEquals([ 'type' => 'expr', 'expr' => 'foodesc + barasc', - 'direction' => 'ASC' + 'direction' => 'ASC', ], $results[0]); } @@ -66,5 +64,4 @@ public function testCache() $results2 = $analyzer->analyzeOrderBy('foo'); // For code coverage purpose } - } diff --git a/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php b/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php index 7f7962c..4089af7 100644 --- a/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php +++ b/tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php @@ -39,7 +39,6 @@ use Mouf\Database\TDBM\Test\Dao\RoleDao; use Mouf\Database\TDBM\Test\Dao\UserDao; use Mouf\Database\TDBM\Utils\TDBMDaoGenerator; -use PHPSQLParser\PHPSQLParser; /** */ From 8e1ed18df5c66c3f88321d38e90935971f3d874f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Sep 2016 10:51:45 +0200 Subject: [PATCH 3/3] Adding a direct dependency on greenlion/php-sql-parser (because we now directly use it) --- composer.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 8e7ed84..09ac459 100644 --- a/composer.json +++ b/composer.json @@ -32,7 +32,8 @@ "beberlei/porpaginas": "~1.0", "mouf/classname-mapper": "~1.0", "mouf/utils.common.doctrine-cache-wrapper": "~1.0", - "logger/essentials" : "^0.1.9" + "logger/essentials" : "^0.1.9", + "greenlion/php-sql-parser": "^4.0" }, "require-dev" : { "phpunit/phpunit": "~5.5",