From a1cef2864cfb66801c18e4c8b5ac2c0327f3d524 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 28 Sep 2023 15:35:00 +0700 Subject: [PATCH 1/6] Fix cte --- src/QueryBuilder/AbstractDQLQueryBuilder.php | 35 +++++++++++++- tests/AbstractQueryBuilderTest.php | 48 ++++++++++++++++---- tests/Common/CommonQueryTest.php | 43 ++++++++++++++++++ 3 files changed, 115 insertions(+), 11 deletions(-) diff --git a/src/QueryBuilder/AbstractDQLQueryBuilder.php b/src/QueryBuilder/AbstractDQLQueryBuilder.php index 77418e5af..e42a55da3 100644 --- a/src/QueryBuilder/AbstractDQLQueryBuilder.php +++ b/src/QueryBuilder/AbstractDQLQueryBuilder.php @@ -421,7 +421,9 @@ public function buildWithQueries(array $withs, array &$params): string [$with['query'], $params] = $this->build($with['query'], $params); } - $result[] = $with['alias'] . ' AS (' . $with['query'] . ')'; + $quotedAlias = $this->quoteCteAlias($with['alias']); + + $result[] = $quotedAlias . ' AS (' . $with['query'] . ')'; } return 'WITH ' . ($recursive ? 'RECURSIVE ' : '') . implode(', ', $result); @@ -610,4 +612,35 @@ private function quoteTableNames(array $tables, array &$params): array return $tables; } + + /** + * Quotes an alias of Common Table Expressions (CTE) + * + * @param string $name The alias name with or without column names to quote. + * + * @return string The quoted alias. + */ + private function quoteCteAlias(string $name): string + { + if (!str_contains($name, '(')) { + return $this->quoter->quoteTableName($name); + } + + if (!str_ends_with($name, ')')) { + return $name; + } + + /** @psalm-suppress PossiblyUndefinedArrayOffset */ + [$name, $columnNames] = explode('(', substr($name, 0, -1), 2); + + $quotedName = $this->quoter->quoteTableName(trim($name)); + $columnNames = explode(',', $columnNames); + $quotedColumnNames = []; + + foreach ($columnNames as $columnName) { + $quotedColumnNames[] = $this->quoter->quoteColumnName(trim($columnName)); + } + + return $quotedName . '(' . implode(', ', $quotedColumnNames) . ')'; + } } diff --git a/tests/AbstractQueryBuilderTest.php b/tests/AbstractQueryBuilderTest.php index 6a50636e9..c12b6ee74 100644 --- a/tests/AbstractQueryBuilderTest.php +++ b/tests/AbstractQueryBuilderTest.php @@ -668,7 +668,7 @@ public function testBuildWithQueries(): void $this->assertSame( DbHelper::replaceQuotes( <<getDriverName(), ), @@ -1131,7 +1131,7 @@ public function testBuildWithQuery(): void $this->assertSame( DbHelper::replaceQuotes( <<getDriverName(), ), @@ -1157,18 +1157,46 @@ public function testBuildWithQueryRecursive(): void [$sql, $params] = $qb->build($query); - $this->assertSame( - DbHelper::replaceQuotes( - <<getDriverName(), - ), - $sql, + $expected = DbHelper::replaceQuotes( + <<getDriverName(), ); + + if (in_array($db->getDriverName(), ['oci', 'sqlsrv'], true)) { + $expected = str_replace('WITH RECURSIVE ', 'WITH ', $expected); + } + + $this->assertSame($expected, $sql); $this->assertSame([], $params); } + public function testBuildWithQueryRecursiveWithColumns() + { + $db = $this->getConnection(); + + $qb = $db->getQueryBuilder(); + $withQuery = (new Query($db))->select(['id', 'name'])->from('user')->where(['status' => 1]); + $query = (new Query($db))->withQuery($withQuery, 'u(id, name)', true)->from('account'); + + [$sql, $params] = $qb->build($query); + + $expected = DbHelper::replaceQuotes( + <<getDriverName(), + ); + + if (in_array($db->getDriverName(), ['oci', 'sqlsrv'], true)) { + $expected = str_replace('WITH RECURSIVE ', 'WITH ', $expected); + } + + $this->assertSame($expected, $sql); + $this->assertSame([':qp0' => 1], $params); + } + /** * @throws Exception * @throws InvalidConfigException diff --git a/tests/Common/CommonQueryTest.php b/tests/Common/CommonQueryTest.php index 7b098207f..9b1373c06 100644 --- a/tests/Common/CommonQueryTest.php +++ b/tests/Common/CommonQueryTest.php @@ -4,6 +4,7 @@ namespace Yiisoft\Db\Tests\Common; +use Yiisoft\Db\Expression\Expression; use Yiisoft\Db\Query\Query; use Yiisoft\Db\Tests\AbstractQueryTest; @@ -34,4 +35,46 @@ public function testColumnIndexByWithClosure() $db->close(); } + + public function testWithQuery() + { + $db = $this->getConnection(); + + $with = (new Query($db)) + ->distinct() + ->select(['status']) + ->from('customer'); + + $query = (new Query($db)) + ->withQuery($with, 'statuses') + ->from('statuses'); + + $this->assertEquals(2, $query->count()); + } + + public function testWithQueryRecursive() + { + $db = $this->getConnection(); + $quoter = $db->getQuoter(); + $isOracle = $db->getDriverName() === 'oci'; + + /** Sum 1 to 10 equals 55 */ + $quotedName = $quoter->quoteColumnName('n'); + $union = (new Query($db)) + ->select(new Expression($quotedName . ' + 1')) + ->from('t') + ->where(['<', 'n', 10]); + + $with = (new Query($db)) + ->select(new Expression('1')) + ->from($isOracle ? new Expression('DUAL') : []) + ->union($union, true); + + $sum = (new Query($db)) + ->withQuery($with, 't(n)', true) + ->from('t') + ->sum($quotedName); + + $this->assertEquals(55, $sum); + } } From fb6bc5df7fb15b1ca39c3677f308799586870b2b Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 28 Sep 2023 15:51:39 +0700 Subject: [PATCH 2/6] Fix tests --- tests/Common/CommonQueryTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/Common/CommonQueryTest.php b/tests/Common/CommonQueryTest.php index 9b1373c06..a1a6bb1d0 100644 --- a/tests/Common/CommonQueryTest.php +++ b/tests/Common/CommonQueryTest.php @@ -38,7 +38,7 @@ public function testColumnIndexByWithClosure() public function testWithQuery() { - $db = $this->getConnection(); + $db = $this->getConnection(true); $with = (new Query($db)) ->distinct() @@ -50,6 +50,8 @@ public function testWithQuery() ->from('statuses'); $this->assertEquals(2, $query->count()); + + $db->close(); } public function testWithQueryRecursive() @@ -76,5 +78,7 @@ public function testWithQueryRecursive() ->sum($quotedName); $this->assertEquals(55, $sum); + + $db->close(); } } From c8e431d3ef98b842ab817fbd94430305ff49e8b5 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 28 Sep 2023 16:53:40 +0700 Subject: [PATCH 3/6] Add `ExpressionInterface` as alias type and update tests --- src/Query/Query.php | 2 +- src/Query/QueryPartsInterface.php | 5 +-- src/QueryBuilder/AbstractDQLQueryBuilder.php | 21 +++++------- tests/AbstractQueryBuilderTest.php | 22 ++++-------- tests/Provider/QueryBuilderProvider.php | 36 ++++++++++++++++++++ 5 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/Query/Query.php b/src/Query/Query.php index 98e69b962..4ba681e7b 100644 --- a/src/Query/Query.php +++ b/src/Query/Query.php @@ -662,7 +662,7 @@ public function where(array|string|ExpressionInterface|null $condition, array $p return $this; } - public function withQuery(QueryInterface|string $query, string $alias, bool $recursive = false): static + public function withQuery(QueryInterface|string $query, ExpressionInterface|string $alias, bool $recursive = false): static { $this->withQueries[] = ['query' => $query, 'alias' => $alias, 'recursive' => $recursive]; return $this; diff --git a/src/Query/QueryPartsInterface.php b/src/Query/QueryPartsInterface.php index f6b9ca974..8c3ddfd84 100644 --- a/src/Query/QueryPartsInterface.php +++ b/src/Query/QueryPartsInterface.php @@ -645,10 +645,11 @@ public function where(array|string|ExpressionInterface|null $condition, array $p * Prepends an SQL statement using `WITH` syntax. * * @param QueryInterface|string $query The SQL statement to append using `UNION`. - * @param string $alias The query alias in `WITH` construction. + * @param ExpressionInterface|string $alias The query alias in `WITH` construction. + * To specify the alias in plain SQL, you may pass an instance of {@see ExpressionInterface}. * @param bool $recursive Its `true` if using `WITH RECURSIVE` and `false` if using `WITH`. */ - public function withQuery(QueryInterface|string $query, string $alias, bool $recursive = false): static; + public function withQuery(QueryInterface|string $query, ExpressionInterface|string $alias, bool $recursive = false): static; /** * Specifies the `WITH` query clause for the query. diff --git a/src/QueryBuilder/AbstractDQLQueryBuilder.php b/src/QueryBuilder/AbstractDQLQueryBuilder.php index e42a55da3..014656ebf 100644 --- a/src/QueryBuilder/AbstractDQLQueryBuilder.php +++ b/src/QueryBuilder/AbstractDQLQueryBuilder.php @@ -616,12 +616,16 @@ private function quoteTableNames(array $tables, array &$params): array /** * Quotes an alias of Common Table Expressions (CTE) * - * @param string $name The alias name with or without column names to quote. + * @param ExpressionInterface|string $name The alias name with or without column names to quote. * * @return string The quoted alias. */ - private function quoteCteAlias(string $name): string + private function quoteCteAlias(ExpressionInterface|string $name): string { + if ($name instanceof ExpressionInterface) { + return $this->buildExpression($name); + } + if (!str_contains($name, '(')) { return $this->quoter->quoteTableName($name); } @@ -631,16 +635,9 @@ private function quoteCteAlias(string $name): string } /** @psalm-suppress PossiblyUndefinedArrayOffset */ - [$name, $columnNames] = explode('(', substr($name, 0, -1), 2); - - $quotedName = $this->quoter->quoteTableName(trim($name)); - $columnNames = explode(',', $columnNames); - $quotedColumnNames = []; - - foreach ($columnNames as $columnName) { - $quotedColumnNames[] = $this->quoter->quoteColumnName(trim($columnName)); - } + [$name, $columns] = explode('(', substr($name, 0, -1), 2); + $name = trim($name); - return $quotedName . '(' . implode(', ', $quotedColumnNames) . ')'; + return $this->quoter->quoteTableName($name) . '(' . $this->buildColumns($columns) . ')'; } } diff --git a/tests/AbstractQueryBuilderTest.php b/tests/AbstractQueryBuilderTest.php index c12b6ee74..5b72f2694 100644 --- a/tests/AbstractQueryBuilderTest.php +++ b/tests/AbstractQueryBuilderTest.php @@ -1172,29 +1172,21 @@ public function testBuildWithQueryRecursive(): void $this->assertSame([], $params); } - public function testBuildWithQueryRecursiveWithColumns() + /** @dataProvider \Yiisoft\Db\Tests\Provider\QueryBuilderProvider::cteAliases */ + public function testBuildWithQueryAlias($alias, $expected) { $db = $this->getConnection(); - $qb = $db->getQueryBuilder(); - $withQuery = (new Query($db))->select(['id', 'name'])->from('user')->where(['status' => 1]); - $query = (new Query($db))->withQuery($withQuery, 'u(id, name)', true)->from('account'); - [$sql, $params] = $qb->build($query); + $withQuery = (new Query($db))->from('t'); + $query = (new Query($db))->withQuery($withQuery, $alias)->from('t'); - $expected = DbHelper::replaceQuotes( - <<getDriverName(), - ); + [$sql, $params] = $qb->build($query); - if (in_array($db->getDriverName(), ['oci', 'sqlsrv'], true)) { - $expected = str_replace('WITH RECURSIVE ', 'WITH ', $expected); - } + $expected = DbHelper::replaceQuotes($expected, $db->getDriverName()); $this->assertSame($expected, $sql); - $this->assertSame([':qp0' => 1], $params); + $this->assertSame([], $params); } /** diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index f86d147d9..bb09c3e1a 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -1231,4 +1231,40 @@ public static function upsert(): array ], ]; } + + public static function cteAliases(): array + { + return [ + 'simple' => [ + 'a', + << [ + 'a(b)', + << [ + 'a(b,c,d)', + << [ + 'a(b,c,d) ', + << [ + new Expression('a(b,c,d)'), + << Date: Thu, 28 Sep 2023 17:16:01 +0700 Subject: [PATCH 4/6] Update psalm --- src/QueryBuilder/AbstractDQLQueryBuilder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/QueryBuilder/AbstractDQLQueryBuilder.php b/src/QueryBuilder/AbstractDQLQueryBuilder.php index 014656ebf..9f857328f 100644 --- a/src/QueryBuilder/AbstractDQLQueryBuilder.php +++ b/src/QueryBuilder/AbstractDQLQueryBuilder.php @@ -411,7 +411,7 @@ public function buildWithQueries(array $withs, array &$params): string $recursive = false; $result = []; - /** @psalm-var array $withs */ + /** @psalm-var array{query:string|Query, alias:ExpressionInterface|string, recursive:bool}[] $withs */ foreach ($withs as $with) { if ($with['recursive']) { $recursive = true; From 4daf4aad2ba3b5932488654001d2689ef0de64b8 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 28 Sep 2023 17:32:14 +0700 Subject: [PATCH 5/6] Update tests --- tests/AbstractQueryBuilderTest.php | 9 +++++-- tests/Provider/QueryBuilderProvider.php | 35 ++++--------------------- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/tests/AbstractQueryBuilderTest.php b/tests/AbstractQueryBuilderTest.php index 5b72f2694..31de64c71 100644 --- a/tests/AbstractQueryBuilderTest.php +++ b/tests/AbstractQueryBuilderTest.php @@ -1183,9 +1183,14 @@ public function testBuildWithQueryAlias($alias, $expected) [$sql, $params] = $qb->build($query); - $expected = DbHelper::replaceQuotes($expected, $db->getDriverName()); + $expectedSql = DbHelper::replaceQuotes( + <<getDriverName(), + ); - $this->assertSame($expected, $sql); + $this->assertSame($expectedSql, $sql); $this->assertSame([], $params); } diff --git a/tests/Provider/QueryBuilderProvider.php b/tests/Provider/QueryBuilderProvider.php index bb09c3e1a..0871a113b 100644 --- a/tests/Provider/QueryBuilderProvider.php +++ b/tests/Provider/QueryBuilderProvider.php @@ -1235,36 +1235,11 @@ public static function upsert(): array public static function cteAliases(): array { return [ - 'simple' => [ - 'a', - << [ - 'a(b)', - << [ - 'a(b,c,d)', - << [ - 'a(b,c,d) ', - << [ - new Expression('a(b,c,d)'), - << ['a', '[[a]]'], + 'with one column' => ['a(b)', '[[a]]([[b]])'], + 'with columns' => ['a(b,c,d)', '[[a]]([[b]], [[c]], [[d]])'], + 'with extra space' => ['a(b,c,d) ', 'a(b,c,d) '], + 'expression' => [new Expression('a(b,c,d)'), 'a(b,c,d)'], ]; } } From d3626378acef39a58b5df8675252fe8b8faf763c Mon Sep 17 00:00:00 2001 From: Tigrov Date: Thu, 28 Sep 2023 18:04:32 +0700 Subject: [PATCH 6/6] Add line to CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb84eb0d1..0f4e9b299 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Bug #756: Fix `Quoter::quoteSql()` for SQL containing table with prefix (@Tigrov) - Bug #756: Fix `Quoter::getTableNameParts()` for cases when different quotes for tables and columns (@Tigrov) - Bug #756: Fix `Quoter::quoteTableName()` for sub-query with alias (@Tigrov) +- Bug #761: Quote aliases of CTE in `WITH` queries (@Tigrov) ## 1.1.1 August 16, 2023