From 6c56e6a3a7a6b7efae78b2d655e5236452192da6 Mon Sep 17 00:00:00 2001 From: kharhamel Date: Tue, 21 Apr 2020 16:59:55 +0200 Subject: [PATCH 1/2] created a unit test for the bug: count doesn't work when fetching objects from a findFromRawSQL on inherited tables --- .gitignore | 3 ++- tests/Dao/TestPersonDao.php | 30 ++++++++++++++++++++++++++++++ tests/TDBMDaoGeneratorTest.php | 10 ++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/Dao/TestPersonDao.php diff --git a/.gitignore b/.gitignore index f4f7f89d..88b767a7 100644 --- a/.gitignore +++ b/.gitignore @@ -13,4 +13,5 @@ vendor/* /.travis/ /vendor-bin/require-checker/vendor/ /vendor-bin/couscous/vendor/ -.phpunit.result.cache \ No newline at end of file +.phpunit.result.cache +tdbm.lock.yml \ No newline at end of file diff --git a/tests/Dao/TestPersonDao.php b/tests/Dao/TestPersonDao.php new file mode 100644 index 00000000..533fef8b --- /dev/null +++ b/tests/Dao/TestPersonDao.php @@ -0,0 +1,30 @@ +findFromRawSql($sql, []); + } +} \ No newline at end of file diff --git a/tests/TDBMDaoGeneratorTest.php b/tests/TDBMDaoGeneratorTest.php index 86a5bbb9..b4d0da7a 100644 --- a/tests/TDBMDaoGeneratorTest.php +++ b/tests/TDBMDaoGeneratorTest.php @@ -32,6 +32,7 @@ use ReflectionMethod; use TheCodingMachine\TDBM\Dao\TestArticleDao; use TheCodingMachine\TDBM\Dao\TestCountryDao; +use TheCodingMachine\TDBM\Dao\TestPersonDao; use TheCodingMachine\TDBM\Dao\TestRoleDao; use TheCodingMachine\TDBM\Dao\TestUserDao; use TheCodingMachine\TDBM\Fixtures\Interfaces\TestUserDaoInterface; @@ -2178,4 +2179,13 @@ public function testFindByDateTime(): void $personDao->findByModifiedAt(new \DateTimeImmutable())->count(); $this->assertTrue(true); } + + public function testFindFromRawSQLOnInheritance(): void + { + $dao = new TestPersonDao($this->tdbmService); + $objects = $dao->testFindFromRawSQLONInherited(); + + $this->assertNotNull($objects->first()); + $this->assertNotEquals(0, $objects->count()); + } } From 9e04377bff6f08f9ed09f882b2becba5602cb40d Mon Sep 17 00:00:00 2001 From: Guillaume Date: Fri, 24 Apr 2020 16:07:02 +0200 Subject: [PATCH 2/2] findFromRawSql: fix COUNT with DISTINCT --- .../FindObjectsFromRawSqlQueryFactory.php | 29 ++++++++++++---- tests/Dao/TestAlbumDao.php | 33 +++++++++++++++++++ tests/Dao/TestPersonDao.php | 5 ++- tests/TDBMDaoGeneratorTest.php | 21 ++++++++++-- 4 files changed, 77 insertions(+), 11 deletions(-) create mode 100644 tests/Dao/TestAlbumDao.php diff --git a/src/QueryFactory/FindObjectsFromRawSqlQueryFactory.php b/src/QueryFactory/FindObjectsFromRawSqlQueryFactory.php index 0ace6d02..f2ed0e63 100644 --- a/src/QueryFactory/FindObjectsFromRawSqlQueryFactory.php +++ b/src/QueryFactory/FindObjectsFromRawSqlQueryFactory.php @@ -144,14 +144,16 @@ private function processParsedUnionQuery(array $parsedSql, ?string $sqlCount): a private function processParsedSelectQuery(array $parsedSql, ?string $sqlCount): array { // 1: let's reformat the SELECT and construct our columns - list($select, $columnDescriptors) = $this->formatSelect($parsedSql['SELECT']); + list($select, $countSelect, $columnDescriptors) = $this->formatSelect($parsedSql['SELECT']); $generator = new PHPSQLCreator(); $parsedSql['SELECT'] = $select; $processedSql = $generator->create($parsedSql); // 2: let's compute the count query if needed if ($sqlCount === null) { - $parsedSqlCount = $this->generateParsedSqlCount($parsedSql); + $parsedCountSql = $parsedSql; + $parsedCountSql['SELECT'] = $countSelect; + $parsedSqlCount = $this->generateParsedSqlCount($parsedCountSql); $processedSqlCount = $generator->create($parsedSqlCount); } else { $processedSqlCount = $sqlCount; @@ -173,40 +175,46 @@ private function formatSelect(array $baseSelect): array $connection = $this->tdbmService->getConnection(); $formattedSelect = []; + $formattedCountSelect = []; $columnDescriptors = []; $fetchedTables = []; foreach ($baseSelect as $entry) { if ($entry['expr_type'] !== 'colref') { $formattedSelect[] = $entry; + $formattedCountSelect[] = $entry; continue; } $noQuotes = $entry['no_quotes']; if ($noQuotes['delim'] !== '.' || count($noQuotes['parts']) !== 2) { $formattedSelect[] = $entry; + $formattedCountSelect[] = $entry; continue; } $tableName = $noQuotes['parts'][0]; if (!in_array($tableName, $relatedTables)) { $formattedSelect[] = $entry; + $formattedCountSelect[] = $entry; continue; } $columnName = $noQuotes['parts'][1]; if ($columnName !== '*') { $formattedSelect[] = $entry; + $formattedCountSelect[] = $entry; continue; } $table = $this->schema->getTable($tableName); + $pkColumns = $table->getPrimaryKeyColumns(); foreach ($table->getColumns() as $column) { $columnName = $column->getName(); $alias = "{$tableName}____{$columnName}"; - $formattedSelect[] = [ + $astColumn = [ 'expr_type' => 'colref', - 'base_expr' => $connection->quoteIdentifier($tableName).'.'.$connection->quoteIdentifier($columnName), + 'base_expr' => $connection->quoteIdentifier($tableName) . '.' . $connection->quoteIdentifier($columnName), 'no_quotes' => [ 'delim' => '.', 'parts' => [ @@ -219,7 +227,10 @@ private function formatSelect(array $baseSelect): array 'name' => $alias, ] ]; - + $formattedSelect[] = $astColumn; + if (in_array($columnName, $pkColumns)) { + $formattedCountSelect[] = $astColumn; + } $columnDescriptors[$alias] = [ 'as' => $alias, 'table' => $tableName, @@ -241,7 +252,13 @@ private function formatSelect(array $baseSelect): array $formattedSelect[$i]['delim'] = ','; } } - return [$formattedSelect, $columnDescriptors]; + + for ($i = 0; $i < count($formattedCountSelect) - 1; $i++) { + if (!isset($formattedCountSelect[$i]['delim'])) { + $formattedCountSelect[$i]['delim'] = ','; + } + } + return [$formattedSelect, $formattedCountSelect, $columnDescriptors]; } /** diff --git a/tests/Dao/TestAlbumDao.php b/tests/Dao/TestAlbumDao.php new file mode 100644 index 00000000..5abf59d9 --- /dev/null +++ b/tests/Dao/TestAlbumDao.php @@ -0,0 +1,33 @@ +findFromRawSql('SELECT DISTINCT albums.* FROM albums'); + } + + /** + * @return \TheCodingMachine\TDBM\ResultIterator|AlbumBean[] + */ + public function findAllFromRawSqlWithCount() + { + return $this->findFromRawSql( + 'SELECT DISTINCT albums.* FROM albums', + [], + 'SELECT COUNT(DISTINCT albums.id) FROM albums' + ); + } +} diff --git a/tests/Dao/TestPersonDao.php b/tests/Dao/TestPersonDao.php index 533fef8b..a4581ee0 100644 --- a/tests/Dao/TestPersonDao.php +++ b/tests/Dao/TestPersonDao.php @@ -16,15 +16,14 @@ */ class TestPersonDao extends PersonBaseDao { - public function testFindFromRawSQLONInherited(): ResultIterator + public function testFindFromRawSQLOnInherited(): ResultIterator { $sql = ' SELECT DISTINCT person.*, contact.*, users.* FROM person JOIN contact ON person.id = contact.id JOIN users ON contact.id = users.id - WHERE TRUE '; return $this->findFromRawSql($sql, []); } -} \ No newline at end of file +} diff --git a/tests/TDBMDaoGeneratorTest.php b/tests/TDBMDaoGeneratorTest.php index b4d0da7a..ef6281a9 100644 --- a/tests/TDBMDaoGeneratorTest.php +++ b/tests/TDBMDaoGeneratorTest.php @@ -30,6 +30,7 @@ use Ramsey\Uuid\Uuid; use ReflectionClass; use ReflectionMethod; +use TheCodingMachine\TDBM\Dao\TestAlbumDao; use TheCodingMachine\TDBM\Dao\TestArticleDao; use TheCodingMachine\TDBM\Dao\TestCountryDao; use TheCodingMachine\TDBM\Dao\TestPersonDao; @@ -43,6 +44,7 @@ use TheCodingMachine\TDBM\Test\Dao\ArtistDao; use TheCodingMachine\TDBM\Test\Dao\BaseObjectDao; use TheCodingMachine\TDBM\Test\Dao\Bean\AccountBean; +use TheCodingMachine\TDBM\Test\Dao\Bean\AlbumBean; use TheCodingMachine\TDBM\Test\Dao\Bean\AllNullableBean; use TheCodingMachine\TDBM\Test\Dao\Bean\AnimalBean; use TheCodingMachine\TDBM\Test\Dao\Bean\Article2Bean; @@ -2180,12 +2182,27 @@ public function testFindByDateTime(): void $this->assertTrue(true); } + /** + * Bug: find from sql use a `COUNT(DISTINCT *)` which fails because of null values. + */ + public function testFindFromRawSqlCount(): void + { + $dao = new TestAlbumDao($this->tdbmService); + $albums = $dao->findAllFromRawSql(); + + $firstAlbum = $albums->first(); + assert($firstAlbum instanceof AlbumBean); + $this->assertNull($firstAlbum->getNode()); // This null ensure reproducibility of the bug + $expectedCount = $dao->findAllFromRawSqlWithCount()->count(); + $this->assertEquals($expectedCount, $albums->count()); + } + public function testFindFromRawSQLOnInheritance(): void { $dao = new TestPersonDao($this->tdbmService); - $objects = $dao->testFindFromRawSQLONInherited(); + $objects = $dao->testFindFromRawSQLOnInherited(); $this->assertNotNull($objects->first()); - $this->assertNotEquals(0, $objects->count()); + $this->assertEquals(6, $objects->count()); } }